Skip to content

fix: Prevent a full status listener from hanging Close - #498

Merged
beekld merged 6 commits into
v7from
bklimt/SDK-3425/broadcaster-unblock
Nov 3, 2026
Merged

beekld merged 6 commits into
v7from
bklimt/SDK-3425/broadcaster-unblock

Conversation

@beekld

@beekld beekld commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Summary

#489 made FDv2.Stop() broadcast an Off status after its goroutines exit. A broadcast does a blocking send to each listener channel, which buffers 10 values. If an application listener stops reading while its buffer is full, Stop() blocks forever. That hangs LDClient.Close(), and GetStatus() hangs with it, because the send ran under the status lock.

The hazard predates #489. Any status update can block the same way while the data system runs, and the broadcaster holds its lock during the send. So removing the stuck listener also blocks.

This PR:

  • Makes RemoveListener and Close unblock a send to a full listener. Listeners that keep reading still get every value, in order. No broadcast drops events.
  • Sends the FDv2 data source status broadcast after the status lock is released, as FDv1 already does. GetStatus() no longer waits on a listener.
  • Stops broadcasting Off from FDv2.Stop(). The status still changes to Off, and listeners learn of shutdown when their channels close.
  • Removes the drain goroutines in the FDv1 and FDv2 WaitFor that only existed to work around the blocking removal.
  • Makes WaitFor return false when its listener channel closes, in both FDv1 and FDv2. Before, a WaitFor still running at Close() spun on the closed channel until its timeout.

Note

Overview
Fixes hangs when a status listener’s buffered channel is full and the app stops reading—previously Broadcast, RemoveListener, Close, FDv2.Stop(), and GetStatus() could block indefinitely (including LDClient.Close()).

The generic Broadcaster now sends without holding the subscriber list lock, uses per-subscriber done signaling so removal/shutdown aborts in-flight sends, and still delivers every value in order to listeners that keep reading. FDv2.UpdateStatus broadcasts only after the status mutex is released (matching FDv1). Stop() sets status to Off internally but no longer broadcasts Off—shutdown is signaled by closing listener channels. WaitFor drops the old channel-drain workaround and returns false when the listener channel closes (e.g. on stop or broadcaster close).

Tests cover full-buffer broadcast/unblock paths and FDv1/FDv2 WaitFor, GetStatus, and Stop under a full listener.

Reviewed by Cursor Bugbot for commit b5e17cb. Bugbot is set up for automated code reviews on this repo. Configure here.

@beekld
beekld added this pull request to stack #503 November 2, 2026 21:04
@beekld
beekld marked this pull request as ready for review November 2, 2026 21:05
@beekld
beekld requested a review from a team as a code owner November 2, 2026 21:05

@cursor cursor Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Stale Bugbot comment from a previous run.

Comment thread internal/broadcasters.go Resolved
@beekld
beekld force-pushed the bklimt/SDK-3425/broadcaster-unblock branch from 036e2f4 to c1e6e39 Compare November 3, 2026 00:05

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c1e6e39. Configure here.

Comment thread internal/datasystem/fdv2_datasystem.go Resolved

@kinyoklion kinyoklion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approved with consideration of 505.

@tanderson-ld
tanderson-ld self-requested a review November 3, 2026 19:00

@tanderson-ld tanderson-ld left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requires 505 for stack to merge together.

@beekld
beekld force-pushed the bklimt/SDK-3425/broadcaster-unblock branch from c1e6e39 to b5e17cb Compare November 3, 2026 19:38
@beekld
beekld merged commit 9efd570 into v7 Nov 3, 2026
30 checks passed
@beekld
beekld deleted the bklimt/SDK-3425/broadcaster-unblock branch November 3, 2026 21:33
beekld added a commit that referenced this pull request Nov 3, 2026
…ps (#505)

## Summary

**Behavior change:** compared with the released 7.21.2, FDv2 status
listeners that are still reading now receive an `Off` status when the
client closes. Their channels then close as before. 7.21.2 neither
broadcast `Off` nor set it on shutdown. #489
added a blocking broadcast that could hang `Close()`, and
#498 removed it.

This PR adds `Broadcaster.CloseWith`, which sends a final value to each
listener that has room for it in its buffer and then closes every
listener channel. It never waits for a listener to read, so a listener
that has stopped reading still can't block `Close()`. That listener
misses `Off` and only sees its channel close.

The data system uses it in `Stop()` to send the `Off` status. As a
result, a `WaitFor(DataSourceStateOff, ...)` call that's running when
the client closes now returns `true`.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> Adds **`Broadcaster.CloseWith`**, which non-blockingly pushes one
final value into each subscriber channel that still has buffer space,
then closes all listener channels. **`Close()`** is unchanged in
behavior and routes through shared **`closeAll(nil)`** logic;
**`RemoveListener`** still closes without a final value.
> 
> **FDv2 `Stop()`** now records **`DataSourceStateOff`** via
**`maybeUpdateStatus`** and shuts down broadcasters with
**`Close(status)`**, so data-source status listeners that are still
reading typically receive **`Off`** before their channel closes.
**`WaitFor(DataSourceStateOff, …)`** can therefore return **`true`**
when the client stops instead of timing out or failing only on channel
close.
> 
> Listeners whose buffers are full still do not block shutdown—the final
**`Off`** is dropped for them (same non-waiting guarantee as
**`CloseWith`**). Tests cover broadcaster **`CloseWith`** semantics and
FDv2 stop / **`WaitFor(Off)`** behavior.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
0ec9157. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
beekld pushed a commit that referenced this pull request Nov 6, 2026
🤖 I have created a release *beep* *boop*
---


##
[7.22.0](v7.21.2...v7.22.0)
(2026-11-06)


### Features

* Broadcast Off to FDv2 status listeners when the data system stops
([#505](#505))
([5c5b8ce](5c5b8ce))
* End an FDv1 fallback after a default TTL of one hour
([#499](#499))
([1f03a5f](1f03a5f))
* Park FDv2 synchronizers instead of retiring them on unexpected
failures
([#489](#489))
([644253a](644253a))


### Bug Fixes

* Keep data from a store that is not initialized out of override
evaluations and AllFlagsState, and report a store failure
([#491](#491))
([1005a1a](1005a1a))
* Keep the override marking when a migration flag serves an unrecognized
stage ([#492](#492))
([9f9911f](9f9911f))
* Leave StateSince alone when a synchronizer starts
([#502](#502))
([823b9aa](823b9aa))
* Make TestData ValueForAll return its value when targeting is off
([#507](#507))
([7300bc1](7300bc1))
* Park a synchronizer that reports Off or closes its results
([#500](#500))
([f6e9910](f6e9910))
* Poll the override files when they cannot be watched
([#495](#495))
([170ad3b](170ad3b))
* Prevent a full status listener from hanging Close
([#498](#498))
([9efd570](9efd570))
* Say that the data system retries after an unrecoverable HTTP error
([#501](#501))
([d4066cc](d4066cc))
* Treat a repeated override file path in a different case as a repeat on
Windows
([#494](#494))
([eeb68d3](eeb68d3))
* Warn when offline mode skips the override source
([#493](#493))
([69bbe61](69bbe61))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Overview**
> **Cuts the Go server SDK release `7.22.0` and records its notes.**
> 
> The root **`.release-please-manifest.json`** and
**`internal/version.go`** now declare **`7.22.0`** (replacing `7.21.2`).
**`CHANGELOG.md`** gains a new **`[7.22.0]`** section dated
**2026-11-06** documenting shipped work: FDv2 data-system behavior
(status **Off** broadcasts, synchronizer **parking** instead of retire,
**FDv1 fallback TTL**, **StateSince** handling, parking on
**Off**/closed results), plus multiple **bug fixes** around
uninitialized store data in overrides/**AllFlagsState**, migration
override marking, **TestData** when targeting is off, **override file**
polling/deduping on Windows, offline-mode warnings, status listener
**Close** hangs, and messaging for unrecoverable HTTP retries.
> 
> No application code changes appear in this PR; it’s version
bookkeeping and release notes only.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
c763cc0. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

3 participants