Switch from the CRT pull-based HttpRequestBodyStream model to the#6959
Switch from the CRT pull-based HttpRequestBodyStream model to the#6959zoewangg wants to merge 2 commits into
Conversation
push-based HttpStreamBase.writeData() API to avoid potential deadlock issue when request body InputStream blocks.
| <commons-codec.verion>1.17.1</commons-codec.verion> | ||
| <jmh.version>1.37</jmh.version> | ||
| <awscrt.version>0.45.3</awscrt.version> | ||
| <awscrt.version>1.0.0-SNAPSHOT</awscrt.version> |
There was a problem hiding this comment.
This is pending on awslabs/aws-crt-java@main...write-stream to be merged
| while ((read = inputStream.read(buf, 0, buf.length)) >= 0) { | ||
| byte[] chunk = read == buf.length ? buf : Arrays.copyOf(buf, read); | ||
| CompletableFutureUtils.joinInterruptibly(stream.writeData(chunk, false)); | ||
| } |
There was a problem hiding this comment.
when the input stream has no data available, do we just keep looping until we find data? Seems like we will end up with hot looping and wasting CPU loops for this case.
And, a bit optimization, when there is no data, it will be better to skip invoking the writeData, so that less work to be done
There was a problem hiding this comment.
Per InputStream contract, a valid implementation shouldn't return 0 from read() method; it just blocks until data arrives or returns -1 at EOF.
If len is zero, then no bytes are read and 0 is returned; otherwise, there is an attempt to read at least one byte. If no byte is available because the stream is at end of file, the value -1 is returned; otherwise, at least one byte is read and stored into b.
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
force-pushed
the
zoewang/crtWriteStream
branch
from
ba21d05 to
2d49203
Compare
2 participants
Motivation and Context
AwsCrtHttpClientcan potentially deadlock when the request bodyInputStreamblocks on the CRT event loop thread. This can occur when a blocking stream (e.g., aBufferedInputStreamwrapping aResponseInputStream) is used as a request body and the read depends on the same event loop thread to deliver data.CrtRequestInputStreamAdapter.sendRequestBody()is invoked on the CRT event loop thread and callsInputStream.read(), which may block indefinitely. The CRT event loop must never block.Note that this applies to CRT sync HTTP client only and a follow up PR will be created to update async HTTP client
Modifications
HttpRequestBodyStream.sendRequestBody()model to the push-basedHttpStreamBase.writeData()API. The caller thread now writes the request body instead of the event loop thread.CrtRequestExecutor.execute()passesuseManualDataWrites=trueviaacquireStreamwhen the request has a body. Returns anExecutionResultexposing boththe stream future and response future.
AwsCrtHttpClient.CrtHttpRequest.call()waits for stream acquisition, writes the body from the caller thread viastream.writeData(), then waits for theresponse.
CrtRequestInputStreamAdapter(no longer used).aws-crtdependency forwriteDataandacquireStream(request, handler, useManualDataWrites)APIs.Testing
http-clients/aws-crt-clientpass.Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Committhe new file created by the script in
.changes/next-releasewith your changes.LaunchChangelog
License