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

Resolve stopped/received_reset futures on lost connections #1886

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

Ralith
Copy link
Collaborator

@Ralith Ralith commented Jun 5, 2024

If a connection is lost before a stream becomes closed, these would otherwise never complete.

Copy link

@billylindeman billylindeman left a comment

Choose a reason for hiding this comment

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

This resolves an issue where a client connecting to a server can become deadlocked if the server rejects connection with tls error

If a connection is lost before a stream becomes closed, these would
otherwise never complete.
@djc
Copy link
Member

djc commented Jun 6, 2024

Stared at this a little bit in the afternoon but got caught up wondering about why we shouldn't yield the connection errors on any of the other code paths. Is that correct? Why?

@djc
Copy link
Member

djc commented Jun 6, 2024

(In other words I agree that we should not yield Pending on these paths but wasn't sure we should still yield Ok(None) if conn.error is Some.)

@Ralith
Copy link
Collaborator Author

Ralith commented Jun 6, 2024

got caught up wondering about why we shouldn't yield the connection errors on any of the other code paths. Is that correct? Why?

This is intended, though there's room for debate. My general philosophy for accessing state on closed connections is that information that was available immediately before the connection became closed should not be lost/inaccessible after it becomes closed. Hence, if a receive stream is already finished or reset, or a send stream is fully ACKed or stopped, then we continue to expose that information to the application even if the connection has since been lost.

See also RecvStream::poll_read_generic, which only checks conn.error when no further data can be read, allowing previously-received stream data to be drained even after a connection is lost.

@djc djc merged commit 809cf79 into main Jun 7, 2024
8 checks passed
@djc djc deleted the fix-dangling-wakers branch June 7, 2024 04:41
@djc
Copy link
Member

djc commented Jun 7, 2024

Fair!

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