Skip to content

src/client: Make subscribe stream capacity explicit#36

Merged
mxinden merged 4 commits intotestground:masterfrom
mxinden:subscribe-capacity
Aug 30, 2022
Merged

src/client: Make subscribe stream capacity explicit#36
mxinden merged 4 commits intotestground:masterfrom
mxinden:subscribe-capacity

Conversation

@mxinden
Copy link
Copy Markdown
Member

@mxinden mxinden commented Aug 24, 2022

Makes footgun on Client::subscribe explicit.

Note that once the capacity of the returned [Stream] is reached, the
background task blocks and thus all work related to the [Client] will
pause until elements from the [Stream] are consumed and thus capacity
is freed. Callers of [Client::subscribe] should either set a high
capacity, continuously read from the returned [Stream] or drop it.

This has probably cost me 1h to debug on libp2p/test-plans#26. What do folks think?

Makes footgun on `Client::subscribe` explicit.

> Note that once the capacity of the returned [`Stream`] is reached, the
> background task blocks and thus all work related to the [`Client`] will
> pause until elements from the [`Stream`] are consumed and thus capacity
> is freed. Callers of [`Client::subscribe`] should either set a high
> capacity, continuously read from the returned [`Stream`] or drop it.
Copy link
Copy Markdown
Contributor

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. 👍

@laurentsenta
Copy link
Copy Markdown
Contributor

laurentsenta commented Aug 24, 2022

Nice, is this the reason why tests are hanging on failures?

The Testground instance failed with:

Aug 24 04:57:08.754201	INFO	0.6544s      ERROR << single[000] (cbda45) >> thread 'main' panicked at 'a semaphore may not have more than MAX_PERMITS permits (2305843009213693951)', /usr/local/cargo/registry/src/github.zerozr99.workers.dev-1ecc6299db9ec823/tokio-1.20.1/src/sync/batch_semaphore.rs:134:9

https://github.com/testground/sdk-rust/runs/7987701480?check_suite_focus=true#step:10:111

@mxinden
Copy link
Copy Markdown
Member Author

mxinden commented Aug 29, 2022

Nice, is this the reason why tests are hanging on failures?

No. This should not be related to any CI failures.

@mxinden mxinden merged commit 8177242 into testground:master Aug 30, 2022
elenaf9 added a commit to elenaf9/test-plans that referenced this pull request Sep 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants