Skip to content

Default request-reply to Direct mode#1182

Merged
mtmk merged 4 commits into
release/3.0from
flip-requestreply-default-direct
Jun 12, 2026
Merged

Default request-reply to Direct mode#1182
mtmk merged 4 commits into
release/3.0from
flip-requestreply-default-direct

Conversation

@mtmk

@mtmk mtmk commented Jun 8, 2026

Copy link
Copy Markdown
Member

Flip the default request-reply mode to Direct for single-reply requests. Direct avoids per-request subscription setup and channel delivery, correlating replies through the connection's inbox subscription. Direct now honors ThrowIfNoResponders so RequestAsync behaves the same in both modes; the JetStream publish and API paths handle the no-responders sentinel themselves and opt out of the throw.

Fixes #1155

Flip the NatsOpts.RequestReplyMode default from SharedInbox to Direct.
Direct skips per-request subscription setup and channel-based delivery,
correlating replies through the connection's inbox subscription instead.

Direct mode previously ignored ThrowIfNoResponders and always returned the
503 sentinel as a message. Make it honor the flag so RequestAsync behaves
the same in both modes. The JetStream publish and API paths inspect the
sentinel themselves to retry, so they now opt out with
ThrowIfNoResponders=false, matching the shared-inbox path.

Tests that assert exact wire frames, subscription sids, or active
subscription counts are pinned to SharedInbox: Direct opens an inbox
subscription at connect, which shifts those values.
@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @mtmk's task in 4m 15s —— View job


PR Review: Default request-reply to Direct mode

  • Gather context (CLAUDE.md, PR diff, relevant source files)
  • Analyze core changes in ReplyTask.cs and NatsConnection.RequestReply.cs
  • Analyze JetStream changes in NatsJSContext.cs
  • Analyze options changes in NatsOpts.cs
  • Analyze test changes for correctness and coverage
  • Post inline comments on specific issues
  • Write final review summary

Summary

The change is correct and well-structured. The core logic, JetStream opt-outs, and test fixups all look sound. Two minor issues flagged as inline comments:

Correctness

  • ReplyTask.GetResultAsync: the isNoResponders && _throwIfNoResponders check correctly mirrors what NatsSubBase does in the SharedInbox path — 503 sentinel either throws or passes through as a HasNoResponders message depending on the flag. Thread safety is correct: lock covers all three field writes in SetResult, and the TCS signal is the happens-before edge that ensures GetResultAsync reads the written values.
  • NatsConnection.RequestReply.cs: both telemetry and non-telemetry code paths are updated symmetrically. The ?? true null-coalescing is redundant (see inline comment) but not a bug — SetBaseReplyOptsDefaults already ensures the value is non-null before this point.
  • JetStream opt-outs: both TryPublishAsync and TryJSRequestAsync correctly pass ThrowIfNoResponders = false in Direct mode so the 503 sentinel is returned as a message and handled by the existing retry/inspect logic, matching the SharedInbox path behaviour exactly.
  • Cancellation: the test change from ThrowsAsync<OperationCanceledException> to ThrowsAnyAsync<OperationCanceledException> is correct — Task.WaitAsync (used in Direct mode) throws TaskCanceledException, which is a subtype.

Test fixups

All protocol-frame-counting tests that assert specific SUB/SID sequences now force RequestReplyMode = SharedInbox. The comments explaining why are accurate: in Direct mode the connection opens a connection-level inbox subscription at connect time, which would shift SID allocation and add an extra SUB frame. These are good, targeted fixes.

Test duplication (see inline): Default_mode_is_direct_test is behaviourally identical to the existing Direct_request_reply_test; an explicit Assert.Equal(NatsRequestReplyMode.Direct, nats.Opts.RequestReplyMode) would sharpen the "testing the default" intent.

Breaking change note: the default flip from SharedInboxDirect is intentional for the 3.0 release branch. Existing users who relied on the old default will need to opt back in via RequestReplyMode = NatsRequestReplyMode.SharedInbox.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

Comment thread src/NATS.Client.Core/NatsConnection.RequestReply.cs
Comment thread tests/NATS.Client.Core2.Tests/RequestReplyTest.cs
mtmk added 3 commits June 8, 2026 13:32
Honoring ThrowIfNoResponders in Direct mode (previous commit) changed
no-responders from "return a message" to "throw" for callers that had
intentionally selected Direct. Record whether Direct was set explicitly
(vs inherited as the new default) and default such requests to not throw,
restoring their pre-3.x behavior. Default and SharedInbox connections keep
throwing.

The flag is captured in the RequestReplyMode init accessor; the field
initializer bypasses it, so an unset mode is treated as the throwing
default. RequestManyAsync always uses the shared inbox path and is scoped
out of the opt-out. A per-call NatsSubOpts.ThrowIfNoResponders still wins.
MaxParallelThreads was 1.0x (one thread per core), so collections ran
concurrently. Suites that assert on process-global state, notably the
OpenTelemetry tests whose ActivityListener and MeterListener observe
every NATS.Net activity and measurement in the process, then see another
collection's activities and counters and fail with false positives.
Defaulting request-reply to Direct made this worse by routing JetStream
publish through the instrumented RequestAsync path.

Run collections serially across all test projects.
@mtmk
mtmk requested a review from scottf June 9, 2026 12:18

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

LGTM

@mtmk
mtmk merged commit 6962839 into release/3.0 Jun 12, 2026
30 of 31 checks passed
@mtmk
mtmk deleted the flip-requestreply-default-direct branch June 12, 2026 11:41
@mtmk mtmk linked an issue Jun 15, 2026 that may be closed by this pull request
@mtmk mtmk mentioned this pull request Jun 15, 2026
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.

Flip default RequestReplyMode to Direct in 3.x

2 participants