Skip to content

preserve error from cancelled SendStream during cancellation and closing#4882

Merged
marten-seemann merged 3 commits intomasterfrom
send-stream-error
Jan 21, 2025
Merged

preserve error from cancelled SendStream during cancellation and closing#4882
marten-seemann merged 3 commits intomasterfrom
send-stream-error

Conversation

@marten-seemann
Copy link
Copy Markdown
Member

Fixes #4808.

@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 97.82609% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.32%. Comparing base (0fd2dec) to head (3227966).
Report is 14 commits behind head on master.

Files with missing lines Patch % Lines
send_stream.go 97.83% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4882      +/-   ##
==========================================
+ Coverage   83.36%   84.32%   +0.95%     
==========================================
  Files         151      151              
  Lines       16217    16636     +419     
==========================================
+ Hits        13519    14027     +508     
+ Misses       2161     2064      -97     
- Partials      537      545       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment thread send_stream.go Outdated
Comment thread send_stream.go Outdated
@@ -426,13 +427,13 @@ func (s *sendStream) CancelWrite(errorCode StreamErrorCode) {

func (s *sendStream) cancelWriteImpl(errorCode qerr.StreamErrorCode, remote bool) {
Copy link
Copy Markdown
Collaborator

@sukunrt sukunrt Jan 17, 2025

Choose a reason for hiding this comment

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

cancel after close will change the error on stream.Write from "write after close" to "stream reset".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, that's a feature, not a bug :)

You can cancel a stream after calling closing it. This will actually send a RESET_STREAM frame, and addition to the FIN that might already have been sent out.

The reason for this is that this cancels retransmissions for STREAM frames that might be still in flight.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Let me add some more documentation for that, this is kind of unexpected.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done in 1c23ade.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The reason for this is that this cancels retransmissions for STREAM frames that might be still in flight.

This only applies if the FIN hasn't been ACKed, right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It doesn't. There could be outstanding data before the FIN that you might want to cancel.

@marten-seemann marten-seemann merged commit 5d4835e into master Jan 21, 2025
@marten-seemann marten-seemann deleted the send-stream-error branch January 21, 2025 09:44
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.

calling Close on a SendStream after cancelation should not change the error returned by Write

2 participants