Skip to content

Switch CRT HTTP clients (sync and async) from pull-based body to push-based writeData#6993

Open
zoewangg wants to merge 6 commits into
masterfrom
zoewang/crtWriteStreamAsync
Open

Switch CRT HTTP clients (sync and async) from pull-based body to push-based writeData#6993
zoewangg wants to merge 6 commits into
masterfrom
zoewang/crtWriteStreamAsync

Conversation

@zoewangg
Copy link
Copy Markdown
Contributor

@zoewangg zoewangg commented May 27, 2026

Motivation and Context

AwsCrtHttpClient and AwsCrtAsyncHttpClient can deadlock when the request body delivers data on or via the CRT event loop thread. The
CRT event loop must never block.

  • Sync: CrtRequestInputStreamAdapter.sendRequestBody() is invoked on the CRT event loop thread and calls InputStream.read(), which
    may block indefinitely if the stream wraps a source that depends on the same event loop thread to make progress (e.g., a
    BufferedInputStream wrapping a ResponseInputStream).
  • Async: CrtRequestBodyAdapter.sendRequestBody() is invoked on the CRT event loop thread and pulls from a
    ByteBufferStoringSubscriber whose subscription is also driven on the event loop. A user-supplied Publisher<ByteBuffer> that schedules
    onNext back onto the same event loop creates the same deadlock risk.

Modifications

Migrate from CRT's pull-based body API to the push-based writeData API

  • Use acquireStream(request, handler, useManualDataWrites=true) so CRT no longer pulls body bytes from the SDK on the event loop.
  • Body bytes are pushed via HttpStreamBase.writeData(byte[], boolean) from outside the event loop (sync caller thread, or a Reactive
    Streams subscriber on the async path).
  • Sync caller thread writes the body via streamHandler.writeData() after waitForStream(). Async path drives a new
    CrtRequestBodyPublisherSubscriber that pushes each onNext chunk through writeData and uses request(1) for backpressure (next chunk
    is requested only after the previous writeData future completes).
  • Both executors call stream.activate() themselves before completing the SDK-side stream future. CRT's stream manager activates the
    stream after complete(stream) returns; without our own activate, a synchronously chained writeData call would fail with
    AWS_ERROR_HTTP_STREAM_NOT_ACTIVATED. The C-side activate is idempotent.

New shared CrtStreamHandler

  • Encapsulates the CRT stream lifecycle (acquire → activate → write → release/close) and provides a single CAS-guarded
    tryNotifyResponseHandlerError(handler, t) so the response side and request-body subscriber don't double-deliver onError.
  • Constructed with the CompletableFuture<HttpStreamBase> returned (in spirit) by acquireStream. waitForStream() blocks for sync
    callers; writeData(...) chains on stream readiness for async callers.
  • releaseConnection, closeConnection, incrementWindow are silent no-ops if the stream isn't ready (failed-acquire paths).

Defensive completion of futures

  • Synchronous exceptions that previously stranded futures (e.g., a JNI throw from s.writeData, a publisher whose subscribe throws,
    activate throwing) now reliably fail the corresponding future and tear down the stream so callers see a clear error instead of hanging.

Other

  • Delete CrtRequestInputStreamAdapter and CrtRequestBodyAdapter (no longer used).
  • Add Reactive Streams TCK SubscriberBlackboxVerification for CrtRequestBodyPublisherSubscriber (and the surefire/TestNG plumbing in
    the module pom needed to run it). The TCK exposed two real spec violations that are also fixed: rule 2.3 (onError/onComplete MUST NOT
    call Subscription methods) and rule 2.5 (a second onSubscribe MUST be cancelled).
  • Bump aws-crt dependency for writeData and acquireStream(request, handler, useManualDataWrites) APIs.

Testing

  • All aws-crt-client unit/functional tests pass (~190 existing + new tests for the subscriber, stream-handler error paths, executor
    defensive paths, and TCK).
  • Long-running stability tests pass for both sync and async clients.
  • Original deadlock scenario for both sync and async no longer reproduces against S3 under high concurrency.

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 new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the
    instructions. Commit the new file created by the script in .changes/next-release with your changes.
  • 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

zoewangg added 5 commits May 12, 2026 09:47
   push-based HttpStreamBase.writeData() API to avoid potential deadlock issue when request body InputStream blocks.
   Move stream lifecycle operations (writeData, incrementWindow,
   releaseConnection, closeConnection) from ResponseHandlerHelper into
   a dedicated CrtStreamHandler class. This separates header parsing
   from stream management and makes the shared stream guard explicit
   between the request executor and response handler.

   ResponseHandlerHelper now only handles response header parsing.
@zoewangg zoewangg requested a review from a team as a code owner May 27, 2026 00:07
@zoewangg zoewangg force-pushed the zoewang/crtWriteStreamAsync branch from 14bc392 to 7acd52f Compare May 27, 2026 18:08
@zoewangg zoewangg changed the title Switch from the CRT pull-based HttpRequestBodyStream to push model Switch CRT HTTP clients (sync and async) from pull-based body to push-based writeData May 27, 2026
@zoewangg zoewangg force-pushed the zoewang/crtWriteStreamAsync branch from 7acd52f to 0b4a1bf Compare May 28, 2026 03:58
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