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

Document how to close a stream correctly #1493

Merged
merged 10 commits into from
Mar 1, 2023
Merged

Conversation

flub
Copy link
Contributor

@flub flub commented Feb 16, 2023

It is not trivial to correctly close a stream without errors and knowing everything is fully acknowledged. Describe some details around this.

djc
djc previously approved these changes Feb 16, 2023
Copy link
Collaborator

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great! I'll give @Ralith a chance to review as well.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
@flub
Copy link
Contributor Author

flub commented Feb 16, 2023

Good call on actually compiling the example, there were quite a few errors!

I've left some EOF wording in there since I think that is the terminology when using stream I think. Let me know if you'd still like to remove this.

quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
@flub
Copy link
Contributor Author

flub commented Feb 17, 2023

Please take another look, I've adopted the suggested wording changes.

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks!

quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
@flub flub requested a review from Ralith February 26, 2023 10:07
Ralith
Ralith previously approved these changes Feb 26, 2023
flub and others added 7 commits February 27, 2023 11:39
It is not trivial to correctly close a stream without errors and
knowing everything is fully acknowledged.  Describe some details
around this.
Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>
Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>
quinn/src/recv_stream.rs Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Saunders <ben.e.saunders@gmail.com>
Ralith
Ralith previously approved these changes Feb 27, 2023
/// let mut buf = [0u8; 10];
/// let data = recv_stream.read_exact(&mut buf).await?;
/// if recv_stream.read_to_end(0).await.is_err() {
/// recv_stream.stop(0u8.into()).ok();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, of course this call is impossible because read_to_end takes ownership. Perhaps we should change it to take &mut self...

@flub
Copy link
Contributor Author

flub commented Feb 28, 2023

Apologies for just using the github suggestions and letting CI find the bugs, which is just painful and slow.

I've fixed up the example locally and it all works again. I stuck with .read_to_end(0) and updated the description saying when it calls .stop().

Copy link
Collaborator

@Ralith Ralith left a comment

Choose a reason for hiding this comment

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

Thanks for seeing this through!

@Ralith Ralith merged commit dfc1f33 into quinn-rs:main Mar 1, 2023
@flub flub deleted the close-doc branch March 1, 2023 19:50
@djc djc mentioned this pull request May 8, 2023
3 tasks
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.

None yet

3 participants