Skip to content

Fix regression of PublishOptions getStreamTimeout - #1611

Open
scottf wants to merge 3 commits into
mainfrom
fix-regression-getStreamTimeout
Open

Fix regression of PublishOptions getStreamTimeout#1611
scottf wants to merge 3 commits into
mainfrom
fix-regression-getStreamTimeout

Conversation

@scottf

@scottf scottf commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Fixed regression reported in #1610

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Aug 11, 2026

Copy link
Copy Markdown

Claude finished @scottf's task in 2m 22s —— View job


PR Review

  • Gather context
  • Review PR changes
  • Post findings

Summary

The fix correctly identifies the problem and adds the right call site, but there is a subtle bug in the null-check logic that means options.getStreamTimeout() == null will never be true in practice, causing the fix to silently change the effective timeout for callers who pass PublishOptions without an explicit streamTimeout.


Issue Found: Dead null-check / silent timeout change

File: src/main/java/io/nats/client/impl/NatsJetStream.java, line 158

Duration timeout = options == null || options.getStreamTimeout() == null ? getTimeout() : options.getStreamTimeout();

The intent is clear — fall back to the context's getTimeout() when no per-publish timeout was set. But PublishOptions.Builder always initialises streamTimeout to DEFAULT_TIMEOUT (Options.DEFAULT_CONNECTION_TIMEOUT = 2 s):

// PublishOptions.Builder
Duration streamTimeout = DEFAULT_TIMEOUT;   // never null

// and the setter converts null → DEFAULT_TIMEOUT too
this.streamTimeout = validateDurationNotRequiredGtOrEqZero(timeout, DEFAULT_TIMEOUT);

Because there is no public way to construct a PublishOptions with streamTimeout == null, the branch options.getStreamTimeout() == null is dead code.

Practical effect: whenever options != null, options.getStreamTimeout() is always at least DEFAULT_TIMEOUT (2 s). So any caller who passes PublishOptions for unrelated purposes (message ID, expected sequence, etc.) without touching streamTimeout will now silently publish with a 2-second ack timeout, regardless of the JetStream context's requestTimeout. Before this PR the context's requestTimeout always won; after it a hardcoded 2-second default always wins.

Suggested fix — initialise the builder field to null so that "user did not set a timeout" is distinguishable from "user set 2 s":

// PublishOptions.Builder
Duration streamTimeout = null;            // null = "not set"

public Builder streamTimeout(Duration timeout) {
    // keep existing validation; null stays null → "not set"
    this.streamTimeout = timeout == null ? null : validateDurationNotRequiredGtOrEqZero(timeout, null);
    return this;
}

Then getStreamTimeout() can legitimately return null, and the null-check in NatsJetStream will work as intended. Existing users who relied on the streamTimeout defaulting to 2 s would need the builder's final build() step (or the getter's call-site) to apply the default, but that is cleaner than embedding it in the field initialiser.

Fix this →


Everything else looks correct

  • The Duration import addition is correct and necessary.
  • The options == null guard is correct (callers like publish(subject, body) pass null).
  • The call site is the right place to apply the per-publish override.

}

Message resp = makeInternalRequestResponseRequired(subject, merged, data, getTimeout(), CancelAction.COMPLETE, conn.forceFlushOnRequest);
Duration timeout = options == null || options.getStreamTimeout() == null ? getTimeout() : options.getStreamTimeout();

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

options.getStreamTimeout() == null is dead code: PublishOptions.Builder initialises streamTimeout to DEFAULT_TIMEOUT and the setter also converts null → DEFAULT_TIMEOUT, so the field can never be null. Whenever options != null, this line will always use options.getStreamTimeout() (= 2 s default), ignoring the context's requestTimeout for callers who passed PublishOptions without setting streamTimeout.

To make "user did not set a timeout" distinguishable, initialise Builder.streamTimeout = null and let the null-guard here fall through to getTimeout() when it is unset.

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.

1 participant