Do not set a Close finalizer on accepted streams (#116) - #117
Open
youdie006 wants to merge 1 commit into
Open
Conversation
AcceptStream registered a runtime finalizer that calls Close on the returned *Stream wrapper. The canonical usage stream, _ := session.AcceptStream(); go io.Copy(dst, stream) dispatches to WriteTo promoted from the embedded *stream, so during the copy the outer wrapper is reachable only through io.Copy dead argument. GC then runs the finalizer and closes the stream mid-transfer, and the remaining frames are silently dropped on the accepted stream. OpenStream already disabled the identical finalizer for issue #997; remove it from AcceptStream too (and the now-unused runtime import). Add a regression test that copies a large payload out of an accepted stream under GC pressure and asserts nothing is lost. Fixes xtaci#116
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #116.
Problem
AcceptStreamregisters aruntimefinalizer that callsCloseon the returned*Streamwrapper:The canonical server usage is
stream, _ := session.AcceptStream(); go io.Copy(dst, stream).io.Copydispatches toWriteTo, which is promoted from the embedded*stream, so during the copy the outer*Streamwrapper is reachable only throughio.Copy's now-dead argument. GC then collects the wrapper, runs the finalizer, and closes the stream mid-transfer; the frames that arrive afterwards are silently dropped onrecvLoop's closed-stream path, so the receiver just comes up short with no error.OpenStreamalready has the identical finalizer disabled with the comment// NOTE(x): disabled finalizer for issue #997, soAcceptStreamwas simply left inconsistent.Fix
Remove the finalizer from
AcceptStream(matchingOpenStream), and drop the now-unusedruntimeimport.Test
Added
TestAcceptStreamNoCloseFinalizer: a server accepts a stream and drains it with a barego io.Copy(counter, stream)(so the wrapper is not retained) while the test forces GC, and a client sends 1 MiB. It asserts the server drains all 1 MiB. Red-green verified withgo test: before the fix the server drains only ~224 KiB before the finalizer closes the stream (the test fails at its deadline); after, it drains the full payload in ~30 ms.go build,go vetand the fullgo test ./...suite pass.