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

Add Stream::select2 to support short-circuited select #1422

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

Conversation

king6cong
Copy link
Contributor

@king6cong king6cong commented Jan 16, 2019

As discussed in websockets-rs/rust-websocket#136

Often, we have two streams combined by select:

  1. the steam we care about, e.g. a steam of network events, as in the case of websocket connection.
  2. an auxiliary stream, e.g. stream of timeouts or as in the discussion above, a stream of user inputs.

The stream of network events will complete when there is a network failure or something else, but the auxiliary stream often never completes.

When network steam completes, some actions should be take, such as a retry. But because the combined stream doesn't complete, we have no means to know whether the network steam have completed.

With the added Stream::select2 and short_circuit being true, the returned stream completes when one of the two input stream completes, which is suitable for situations like this.

@king6cong king6cong force-pushed the master branch 2 times, most recently from 3d3f7f1 to c66304c Compare January 16, 2019 13:52
@cramertj
Copy link
Member

When network steam completes, some actions should be take, such as a retry.

The way this API works today, it's impossible to know which stream was the one that completed, which seems like it would be necessary for this sort of retry behavior. Is that not the case?

@king6cong
Copy link
Contributor Author

king6cong commented Jan 17, 2019

it's impossible to know which stream was the one that completed, which seems like it would be necessary for this sort of retry behavior

Yes, we don't know which one completes, maybe we can add this capability, but it is not necessary in this case, because we can retry the combined select as a whole, without knowing which stream competes, especially when the other stream is auxiliary.

As in this case:

https://github.com/websockets-rs/rust-websocket/blob/ad7df37bd9e1b2590017cc24fb48abf1c64cc8f7/examples/async-client.rs#L64

When the websocket server is stopped, an TCP FINfrom the server is received on the client side and ACKed immediately, but:

  • in the complete when both stream completes version of select, client will hold on the TCP connection without a FIN sent. I thinks there are two problems here: 1). We are unaware of the server FIN and continue waiting on a TCP connection from which we can no longer receive any data. 2). This also takes up TCP resource longer than needed.
  • in the complete when any stream completes version of select, client will finish the tcp connection with a FIN immediately. The combined stream completes and we can take action If needed. TCP resource also released.

Details: server (127.0.0.1.2794), client (127.0.0.1.62945)

Server stopped in the complete when both stream completes version of select:

tcpdump
IP 127.0.0.1.2794 > 127.0.0.1.62945: Flags [F.], seq 184, ack 194, win 6376, options [nop,nop,TS val 856206690 ecr 856099198], length 0
IP 127.0.0.1.62945 > 127.0.0.1.2794: Flags [.], ack 185, win 6376, options [nop,nop,TS val 856206690 ecr 856206690], length 0
netstat
tcp4       0      0  127.0.0.1.2794         127.0.0.1.62945        FIN_WAIT_2  408107 146988  12187      0 0x2131 0x00000004
tcp4       0      0  127.0.0.1.62945        127.0.0.1.2794         CLOSE_WAIT  408117 146988  22914      0 0x0122 0x00000000

Server stopped in the complete when any stream completes version of select:

tcpdump
IP 127.0.0.1.2794 > 127.0.0.1.62037: Flags [F.], seq 3288009754, ack 1781025459, win 6376, options [nop,nop,TS val 854119104 ecr 854067110], length 0
IP 127.0.0.1.62037 > 127.0.0.1.2794: Flags [.], ack 1, win 6376, options [nop,nop,TS val 854119104 ecr 854119104], length 0
IP 127.0.0.1.62037 > 127.0.0.1.2794: Flags [F.], seq 1, ack 1, win 6376, options [nop,nop,TS val 854119104 ecr 854119104], length 0
IP 127.0.0.1.2794 > 127.0.0.1.62037: Flags [.], ack 2, win 6376, options [nop,nop,TS val 854119104 ecr 854119104], length 0

@king6cong
Copy link
Contributor Author

@cramertj @alexcrichton ping :)

@cramertj
Copy link
Member

TBH I'm not really sure what to do with this. This seems like a pretty uncommon case to me, and you could replicate this behavior by e.g. using try_select and converting the end of the stream into an error and catching that error. On the other hand, the alternatives are sort of hacky and we don't have a good syntax for ergonomically writing things like this by-hand outside of the crate (e.g. what async/await did for normal futures).

@taiki-e taiki-e added A-stream Area: futures::stream and removed futures-util labels Dec 17, 2020
@414owen
Copy link
Contributor

414owen commented Mar 25, 2022

This looks like it's a duplicate of #1881

@taiki-e looks like a lot of people want this feature :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-stream Area: futures::stream
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants