Repository navigation
fix(connection): do not drop observer events when the sink registers late - #1797
Open
evan188199-tech wants to merge 1 commit into
Open
evan188199-tech wants to merge 1 commit into
evan188199-tech wants to merge 1 commit into
Conversation
…late RegisterSink only queued the sink on a buffered channel, and dispatchEvents selected over that channel and the event channel. When both were ready the select could deliver events first, dropping them on an empty sinks slice. Track sinks in a mutex-protected slice instead so RegisterSink takes effect before it returns, and have the test wait for asynchronous delivery with assert.Eventually instead of asserting once. Fixes cloudflare#1796 Signed-off-by: evan188199-tech <238371241+evan188199-tech@users.noreply.github.com>
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.
Summary
Fixes the flaky
TestGracefulShutdownHTTP2under-racedescribed in #1796. There were two races:Observer.RegisterSinkonly queued the sink on a buffered channel, anddispatchEventsselected over that channel and the event channel. When both were ready, the select could pick the event first and deliver it to an empty sinks slice, silently dropping it. Sinks are now kept in async.RWMutex-protected slice, so registration takes effect beforeRegisterSinkreturns. Delivery iterates over a snapshot taken under the lock, so a sink cannot deadlock by re-entering the observer.TestGracefulShutdownHTTP2now waits for the event withassert.Eventuallyinstead of checking a single time.The public API and
sendEventsemantics are unchanged. Only the stdlibsync/slicespackages are added;go.modis untouched.Tests
TestObserverRegisterSinkThenImmediatelySendEventsas a regression test for race 1.gofmtclean,go vet ./connection/ ./supervisor/passes.go test -race -count=3 ./connection/ -run TestGracefulShutdownHTTP2and the new regression test pass.Fixes #1796
This change was prepared with AI assistance and reviewed before submission.