Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace stream::select to stream_select! #414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

blmarket
Copy link

@blmarket blmarket commented Feb 25, 2024

futures::stream::select will poll streams in a round-robin fashion, which means one stream can be blocked if the other stream does not have item ready.

It replaces select to futures::stream_select! which just poll stream whichever comes first.

Reference: https://docs.rs/futures/latest/futures/stream/fn.select.html
Reference: https://docs.rs/futures/latest/futures/macro.stream_select.html

Note: current master branch is broken and need to apply #406 first.


Demo of bug: https://github.com/blmarket/tower-lsp/blob/buggy-example/tests/buggy.rs

Without patch - it hangs if there are 3 or more logs. It runs okay if we comment out 1 or 2 logs.
With patch - it run okay regardless of number of logs

futures::stream::select will poll streams in a round-robin fashion,
which means one stream can be blocked if the other stream does not have
item ready.

It replaces select to stream_select which just poll stream whichever
comes first.

Reference: https://docs.rs/futures/latest/futures/stream/fn.select.html
Reference: https://docs.rs/futures/latest/futures/macro.stream_select.html
@@ -27,7 +29,7 @@ const MESSAGE_QUEUE_SIZE: usize = 100;
/// This socket handles the server-to-client half of the bidirectional communication stream.
pub trait Loopback {
/// Yields a stream of pending server-to-client requests.
type RequestStream: Stream<Item = Request>;
type RequestStream: Stream<Item = Request> + Unpin;
Copy link
Author

Choose a reason for hiding this comment

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

Note: Breaking change, but it still pass the unit tests + works with stdio. Required by stream_select!

@blmarket
Copy link
Author

3 months with no update, so ping here. will ping again next year.

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.

1 participant