test(sharedtest): make FakeLDClient.Close idempotent - #799
Draft
aaron-zeisler wants to merge 2 commits into
Draft
Conversation
Close() closed CloseCh unconditionally whenever it was non-nil, so any path that closed the same fake client twice (e.g. env teardown racing a re-anchor that already closed the old client) panicked with "close of closed channel" and took down the whole test binary. Guard the close with sync.Once, matching the closeOnce pattern already used elsewhere in the codebase (metrics, streams, autoconfig, bigsegments, filedata, events) for the same purpose.
Adds a doc comment on Close() explaining why it must tolerate a second call, plus a regression test that fails with the original "close of closed channel" panic if the sync.Once guard is ever removed. Also covers concurrent callers under -race, which a plain bool guard would not.
aaron-zeisler
changed the title
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
1 participant
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
Makes
FakeLDClient.Close()idempotent in the shared test harness, fixing an intermittentpanic: close of closed channelthat can take down an entirego testrun.Background
Close()closedCloseChunconditionally whenever it was non-nil, with no guard against being called twice on the same instance. This was observed once as a flake inTestConcurrentKeysRAC_ArrayPatchAddsAndRemovesNonAnchorKeyson thefeat/concurrent-keysbranch; the affectedFakeLDClientstruct andClose()method are identical acrossv8,v9, andfeat/concurrent-keys(the surrounding file differs onv9, which uses a different data-destination API, but the struct and this method are untouched).I was not able to reproduce the panic (dozens of
-raceruns of the specific test and of the fullrelaypackage all passed), and I audited every productionclient.Close()call site ininternal/relayenv/env_context_impl.go— they all delete the client from the map before closing it, or evict the previous reference before installing a new one, so I couldn't find a path that double-closes the same client reference. The actual second closer for the one observed panic remains unidentified.Given that, this fix is intentionally defensive rather than a root-cause fix: it makes the fake tolerate a second
Close()regardless of which caller triggers it, which also brings it in line with production semantics — the real SDK's own components (event_processor,streaming_data_source,Broadcaster) are already idempotent onClose()via the samesync.Once/no-op pattern this uses. If the panic recurs, the original stack trace would help pin down the actual second caller.Changes
closeOnce sync.Once, matching thecloseOnceidiom already used throughout this codebase (metrics, streams, autoconfig, bigsegments, filedata, events).Close()stating the idempotency contract.fake_client_test.gocovering two sequentialClose()calls and N concurrent callers under-race— this test reproduces the original panic if the guard is removed.