Skip to content

Release 3.0.0-preview.10#1190

Merged
mtmk merged 1 commit into
release/3.0from
release-3.0.0-preview.10
Jun 17, 2026
Merged

Release 3.0.0-preview.10#1190
mtmk merged 1 commit into
release/3.0from
release-3.0.0-preview.10

Conversation

@mtmk

@mtmk mtmk commented Jun 16, 2026

Copy link
Copy Markdown
Member

Preview release with JetStream teardown fixes.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 2m 47s —— View job


PR Review: Release 3.0.0-preview.10

Todo:

  • Gather context and understand PR scope
  • Examine PR diff and JetStream teardown fix
  • Review source files for correctness, security, and performance
  • Post review findings

Review

This is a version-bump release PR (preview.9 → preview.10). The substantive change is the JetStream teardown fix landed in the base branch via #1188. Below is my review of that fix.

Fix: NatsJSOrderedPushConsumer.DisposeAsync subscription leak

Correctness — ✅ Sound

The fix adds subscription cleanup in DisposeAsync after both background loops finish (NatsJSOrderedPushConsumer.cs:185-190):

var sub = _sub;
if (sub != null)
{
    await sub.UnsubscribeAsync();
    await sub.DisposeAsync();
}

The ordering is safe. _consumerCreateTask completes before _commandTask is stopped (the shutdown sequence preserves this order), so once _commandTask is awaited, _sub is stable — CreatePushConsumer cannot run and assign a new value to _sub. The volatile field ensures the final value is visible on the calling thread.

Thread safety — ✅ Safe

The _sub field is volatile. Both background loops are fully joined before the read, making the load race-free.

UnsubscribeAsync idempotency — ✅ Safe

NatsSubBase.UnsubscribeAsync guards with a _unsubscribed flag (lock-protected), so calling it explicitly and then having DisposeAsync call it internally is harmless.

Minor findings

1. Missing .ConfigureAwait(false) on new awaits

The existing DisposeAsync calls use .ConfigureAwait(false) consistently (e.g. line 167, 174). The two new awaits don't:

await sub.UnsubscribeAsync();   // missing .ConfigureAwait(false)
await sub.DisposeAsync();       // missing .ConfigureAwait(false)

Not a correctness bug in this context (there's no captured sync context), but it's inconsistent. Fix this →

2. No exception safety between UnsubscribeAsync and DisposeAsync

If UnsubscribeAsync throws, DisposeAsync on the sub will be skipped and DeleteConsumerAsync won't run. In practice UnsubscribeAsync is very robust and this path is unlikely to throw, but a try/finally (or just relying on DisposeAsync which calls UnsubscribeAsync internally) would be safer.

Regression test — ✅ Effective

The test in ObjectStoreTest.cs counts ConnectionOpened invocation list entries via reflection before and after 10 GetBytesAsync calls, and asserts the count is unchanged. This is an effective (if reflection-based) regression guard for this exact leak pattern.


Overall: The fix is correct and well-scoped. The two minor items above are low severity and don't block the release. This PR is ready to merge.

@mtmk
mtmk force-pushed the release-3.0.0-preview.10 branch from 64de4cd to 0639339 Compare June 16, 2026 13:39
@mtmk

mtmk commented Jun 16, 2026

Copy link
Copy Markdown
Member Author

Both minor findings from the earlier review (missing ConfigureAwait(false) and the UnsubscribeAsync/DisposeAsync exception-safety note) are addressed by #1191, now folded into this branch. DisposeAsync calls UnsubscribeAsync internally, so the explicit call was dropped and the single DisposeAsync now uses ConfigureAwait(false).

@claude can you have a quick look?

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 50s —— View job


Todo:


Quick look: #1191 findings verified ✅

Checked NatsJSOrderedPushConsumer.cs lines 185–190 on this branch:

// DisposeAsync unsubscribes internally, so a single call covers both.
var sub = _sub;
if (sub != null)
{
    await sub.DisposeAsync().ConfigureAwait(false);
}

Finding 1 — Missing .ConfigureAwait(false): Fixed. The single DisposeAsync call now has .ConfigureAwait(false) (line 189). ✅

Finding 2 — Exception safety between UnsubscribeAsync and DisposeAsync: Fixed by design. The explicit UnsubscribeAsync call was dropped entirely; DisposeAsync already calls it internally, so there's no gap where a throw on UnsubscribeAsync could skip DisposeAsync. The comment on line 185 explains this clearly. ✅

Both issues are cleanly resolved. LGTM.

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

LGTM

@mtmk
mtmk merged commit 2e43d7e into release/3.0 Jun 17, 2026
31 of 33 checks passed
@mtmk
mtmk deleted the release-3.0.0-preview.10 branch June 17, 2026 10:17
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.

2 participants