Skip to content

Do not set a Close finalizer on accepted streams (#116) - #117

Open
youdie006 wants to merge 1 commit into
xtaci:masterfrom
youdie006:fix/116-acceptstream-finalizer
Open

Do not set a Close finalizer on accepted streams (#116)#117
youdie006 wants to merge 1 commit into
xtaci:masterfrom
youdie006:fix/116-acceptstream-finalizer

Conversation

@youdie006

Copy link
Copy Markdown

Fixes #116.

Problem

AcceptStream registers a runtime finalizer that calls Close on the returned *Stream wrapper:

wrapper := &Stream{stream: stream}
runtime.SetFinalizer(wrapper, func(s *Stream) {
    s.Close()
})

The canonical server usage is stream, _ := session.AcceptStream(); go io.Copy(dst, stream). io.Copy dispatches to WriteTo, which is promoted from the embedded *stream, so during the copy the outer *Stream wrapper is reachable only through io.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 on recvLoop's closed-stream path, so the receiver just comes up short with no error.

OpenStream already has the identical finalizer disabled with the comment // NOTE(x): disabled finalizer for issue #997, so AcceptStream was simply left inconsistent.

Fix

Remove the finalizer from AcceptStream (matching OpenStream), and drop the now-unused runtime import.

Test

Added TestAcceptStreamNoCloseFinalizer: a server accepts a stream and drains it with a bare go 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 with go 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 vet and the full go test ./... suite pass.

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
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.

AcceptStream still sets a finalizer that closes the stream mid-transfer — silent data loss on accepted streams

1 participant