Skip to content

fix(sqs): stop close() busy-loop duplicate sends - #7301

Open
zoewangg wants to merge 2 commits into
masterfrom
zoewang/sqs-batch-manager-close-fix
Open

fix(sqs): stop close() busy-loop duplicate sends#7301
zoewangg wants to merge 2 commits into
masterfrom
zoewang/sqs-batch-manager-close-fix

Conversation

@zoewangg

Copy link
Copy Markdown
Contributor

Motivation and Context

This fixes two shutdown issues in SqsAsyncBatchManager.close(), both in the shared internal base class RequestBatchManager (module services/sqs).

  1. Redundant re-sends of a buffered batch on close. When close() was called while other threads were still submitting on a real asynchronous client, a buffered batch could be sent more than once: close() waited for the in-flight response by re-sending the same batch, because the drain loop stopped only after an asynchronous completion callback cleared it rather than after draining the buffer. The effect was a bounded number of duplicate SendMessageBatch calls for the same messages during that single close(). close() now drains each buffered batch and sends it exactly once, stopping when the buffer is empty (partial batches included).

  2. In-flight requests cancelled on shutdown. Previously close() cancelled in-flight batch sends immediately, so a request that had already been dispatched could complete with CancellationException instead of its real SendMessageResponse. close() now waits a bounded grace period for in-flight sends to finish so their callers receive the real result, and cancels only the requests still outstanding when that period expires. This mirrors the graceful bounded-await shutdown already used by CloudWatchMetricPublisher.close().

Modifications

Drain each buffered batch exactly once:

  • RequestBatchManager now drains each buffer by re-extracting the next batch with extractEntriesForScheduledFlush (the drain-any-non-empty primitive performScheduledFlush already uses) and flushing each distinct batch exactly once, terminating when the buffer is empty. Full and partial batches are now sent exactly once (previously a residual partial batch was never sent and its callers were cancelled).

Graceful bounded-await shutdown (dispatch-all-first, then a single shared wait):

  • RequestBatchManager exposes a two-phase internal shutdown surface: dispatchPending() (phase 1, non-blocking) marks the manager closed, drains/dispatches every buffered batch, and returns a CompletableFuture<Void> that completes when the in-flight sends it dispatched complete; cancelPending() (phase 2) cancels anything still pending and releases the buffers. The wait targets the caller-facing futures (pendingResponses), so a completing send delivers the real result rather than being cancelled.
  • DefaultSqsAsyncBatchManager.close() orchestrates the shutdown: it calls dispatchPending() on all three write managers first (every buffered batch goes on the wire at once), then performs a single bounded wait via CompletableFuture.allOf(...) up to one shared grace period, then calls cancelPending() on all three, then closes the receive manager. Because all three dispatch before the single wait, the whole shutdown is bounded by ONE grace period, not one per manager. The wait helper never throws (handles TimeoutException/ExecutionException/InterruptedException, restoring the interrupt flag).
  • The grace period is a new internal field on RequestBatchConfiguration (@SdkInternalApi), default 5 seconds. There is no public knob.
  • DefaultSqsAsyncBatchManager.close() is guarded by an AtomicBoolean so it is idempotent and safe under concurrent calls: only the first caller runs the dispatch/await/cancel orchestration, and a concurrent or second call is a clean no-op (this avoids a concurrent loser cancelling the winner's still-awaited in-flight sends).
  • RequestBatchManager gains an AtomicBoolean closed guard: a sendMessage / deleteMessage / changeMessageVisibility submitted after close returns a future completed exceptionally with IllegalStateException, matching the receive path.

Documentation:

  • Added an @Override void close() Javadoc on the public SqsAsyncBatchManager documenting that close flushes buffered requests, blocks up to a bounded grace period for in-flight sends so callers receive real results, then cancels stragglers.

Testing

services/sqs built with full static analysis (Checkstyle, SpotBugs, PMD, japicmp) - BUILD SUCCESS, no public API break. New and existing tests pass.

Added new test cases

  • DefaultSqsAsyncBatchManagerTest - close/shutdown behavior against the REAL DefaultSqsAsyncBatchManager (and, through it, the real write batch managers) over a mock SqsAsyncClient with controllable batch-send futures:
  • SqsAsyncBatchManagerTest (WireMock) - real end-to-end batching over HTTP.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds (built the affected module services/sqs, not a full-repo mvn install)
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry.
  • My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

  • I confirm that this pull request can be released under the Apache 2 license

SqsAsyncBatchManager.close() extracted one ready batch once and
re-flushed that same map in a loop that only the async completion
callback could clear, so on a real client the closing thread re-sent
the identical SendMessageBatch thousands of times. It now drains the
buffer by re-extracting and flushing each batch (including partials)
exactly once, terminating when the buffer is empty.

Shutdown is also graceful: DefaultSqsAsyncBatchManager.close() first
dispatches every buffered batch across the three write managers
(dispatchPending), then does a single bounded wait for the in-flight
sends so callers receive real results, and finally cancels only the
stragglers past the deadline (cancelPending). The wait is one shared
grace period (internal, default 5s) across all three managers, not one
each, and close() is idempotent and guarded against concurrent calls.
Post-close submissions fail fast with IllegalStateException. Internal
only; no public API change.
@zoewangg
zoewangg requested a review from a team as a code owner August 21, 2026 17:50
@zoewangg
zoewangg requested a review from Fred1155 August 21, 2026 17:57
Comment thread .changes/next-release/bugfix-AmazonSQS-9b1a920.json Outdated
Renamed the internal shutdown-timeout terminology from "grace period"
to "timeout": DEFAULT_SHUTDOWN_TIMEOUT, the shutdownTimeout field,
accessor, and builder seam, and the SqsAsyncBatchManager.close()
Javadoc. Also renamed RequestBatchManager.dispatchPending() to
closeAndDispatch(). Both per PR review. Updated the changelog wording
accordingly. Internal @SdkInternalApi only; no public API change.
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