Skip to content

Isolate S3 CRT metric publication from event-loop threads - #7422

Merged
jencymaryjoseph merged 4 commits into
feature/master/S3CRTMetricPublisherfrom
jencyjos/s3-crt-metric-publisher-isolation
Oct 30, 2026
Merged

jencymaryjoseph merged 4 commits into
feature/master/S3CRTMetricPublisherfrom
jencyjos/s3-crt-metric-publisher-isolation

Conversation

@jencymaryjoseph

@jencymaryjoseph jencymaryjoseph commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Motivation and context

S3CrtResponseHandlerAdapter.onTelemetry currently invokes customer-provided metric publishers on a CRT AwsEventLoop-* callback thread. Native CRT invokes this callback synchronously, so a slow or blocking publisher can delay other callback work and affect S3 request progress under concurrency.

This change moves MetricPublisher.publish() off CRT callback threads while keeping memory and thread usage bounded.

Modifications

CRT telemetry values are copied into an SDK MetricCollection while the native callback is active. The completed collection and its effective publisher list are then submitted without blocking to a client-scoped asynchronous dispatcher.

The dispatcher uses one daemon worker and a bounded FIFO queue of 128 collections per S3 CRT client. The worker invokes configured publishers sequentially in their configured order. Exceptions are isolated per publisher so they do not prevent remaining publishers or later collections from being processed.

If the queue is full, the dispatcher drops the newly submitted collection for all of its publishers. Queue-full and post-close drops are logged at power-of-two thresholds to avoid log flooding. Because publishers share one worker, a slow publisher can delay other publishers configured for the same client. This shared-queue design keeps the implementation and resource cost bounded without adding per-publisher workers or queues.

Client shutdown remains non-blocking. After native CRT shutdown completes and no more telemetry can be produced, dispatcher shutdown begins asynchronously. Accepted collections are drained for up to five seconds, after which remaining queued collections are discarded and reported. Closing the S3 client does not close customer-provided publishers; their lifecycle remains owned by the caller.

  • Added S3CrtMetricPublisherDispatcher with one bounded queue and daemon worker per S3 CRT client.
  • Submit completed metric collections without blocking CRT callback threads.
  • Preserve collection acceptance order through the FIFO dispatcher.
  • Continue invoking remaining publishers and processing later collections when a publisher throws.
  • Log bounded-queue and lifecycle-related metric drops without flooding logs.
  • Begin asynchronous dispatcher shutdown after native CRT shutdown completes.
  • Drain accepted collections for up to five seconds before discarding queued work.
  • Keep publisher lifecycle ownership with the caller.

Testing

Added unit tests covering callback-thread isolation, daemon execution, ordering, blocking publishers, bounded-queue drops, publisher exception isolation, graceful and timed-out shutdown, asynchronous close, and post-close submissions.

Focused S3 CRT tests passed: 62 tests, 0 failures or errors.

./mvnw -pl :s3 -Dtest=S3CrtMetricPublisherDispatcherTest,S3CrtResponseHandlerAdapterTest,S3CrtResponseHandlerAdapterMetricsTest,S3CrtAsyncHttpClientTest test

Performance benchmarks confirmed that blocking inline publication can significantly increase S3 request latency under concurrency, while asynchronous dispatch protects CRT event-loop processing. The shared queue was selected over per-publisher lanes because publisher-to-publisher isolation is not a requirement and the shared design has lower implementation and resource cost.

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
  • 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 focused 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

@jencymaryjoseph jencymaryjoseph changed the title Jencyjos/s3 crt metric publisher isolation Isolate S3 CRT metric publishers from event loop Oct 2, 2026
@jencymaryjoseph
jencymaryjoseph force-pushed the jencyjos/s3-crt-metric-publisher-isolation branch from e78ec3e to a96f3d6 Compare October 2, 2026 18:04
@jencymaryjoseph
jencymaryjoseph force-pushed the jencyjos/s3-crt-metric-publisher-isolation branch from a96f3d6 to 5790f46 Compare October 14, 2026 05:31
@jencymaryjoseph
jencymaryjoseph marked this pull request as ready for review October 14, 2026 18:47
@jencymaryjoseph
jencymaryjoseph requested a review from a team as a code owner October 14, 2026 18:47
@github-actions

Copy link
Copy Markdown

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 14, 2026
@jencymaryjoseph
jencymaryjoseph force-pushed the jencyjos/s3-crt-metric-publisher-isolation branch from 5790f46 to dd6a1a0 Compare October 26, 2026 22:05
@jencymaryjoseph jencymaryjoseph changed the title Isolate S3 CRT metric publishers from event loop Isolate S3 CRT metric publication from event-loop threads Oct 26, 2026
@aws aws unlocked this conversation Oct 27, 2026
* <p>The SDK does not close these publishers when the client is closed; the caller retains ownership and is
* responsible for closing them.
* responsible for closing them. Metric publication is asynchronous and best effort. Closing the client waits up to
* five seconds for metric collections already accepted by the dispatcher. Collections that arrive while native CRT

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.

Should we remove the internal implemenation details of 5 sec and dispatcher from public java doc

Closing the client blocks for a bounded time while pending metric collections are published.     

}

void startAsyncClose() {
if (asyncCloseStarted.compareAndSet(false, true)) {

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.

nit: I think startAsyncClose() starts a new cleanup thread on every client close, even when no metric was ever dispatched and the executor never created a worker. Can we skip the
thread in that case? Something like:

void startAsyncClose() {                                                                                                                                                           
      if (asyncCloseStarted.compareAndSet(false, true)) {                                                                                                                            
          if (!dispatchAttempted.get()) {                                                                                                                                            
              close(); // nothing queued, so this returns immediately                                                                                                                
              return;                                                                                                                                                                
          }                                                                                                                                                                          
          CLOSE_THREAD_FACTORY.newThread(this::close).start();                                                                                                                       
      }                                                                                                                                                                              
  }                                                                                                                                                                                  
```                                                                                                                                                                            This runs after CRT shutdown completes, so no new telemetry can arrive after the check. 

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Since this runs after CRT shutdown completes, no new telemetry can arrive. I’ll close inline when no metrics were dispatched and only create the cleanup thread when draining may block

@jencymaryjoseph
jencymaryjoseph merged commit c078e86 into feature/master/S3CRTMetricPublisher Oct 30, 2026
5 checks passed
@jencymaryjoseph
jencymaryjoseph deleted the jencyjos/s3-crt-metric-publisher-isolation branch October 30, 2026 19:47
@github-actions

Copy link
Copy Markdown

This pull request has been closed and the conversation has been locked. Comments on closed PRs are hard for our team to see. If you need more assistance, please open a new issue that references this one.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Oct 30, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants