Conversation
| * @param endStream if true, this is the last data to be sent on this stream. | ||
| * @param completionCallback invoked when the data has been flushed or an error occurs. | ||
| */ | ||
| public void writeData(final byte[] data, boolean endStream, |
There was a problem hiding this comment.
I feel like we can also support taking HttpRequestBodyStream, which allows java impl to write directly into C buffer. (SDK probably doesn't use it, but leave the door open) Or we can do that later when we need it.
Meanwhile mention the difference of this and the existing writeChunk method in HttpStream. Looks like we should just mark the writeChunk deprecate?
| /** | ||
| * Schedules an HttpRequestBase on the Native EventLoop for this HttpClientConnection. Applies to both HTTP/2 and HTTP/1.1 connections. | ||
| * | ||
| * @param request The Request to make to the Server. | ||
| * @param streamHandler The Stream Handler to be called from the Native EventLoop | ||
| * @param useManualDataWrites When true, request body data will be provided via | ||
| * {@link HttpStreamBase#writeData} instead of from the request's body stream. | ||
| * @throws CrtRuntimeException if stream creation fails | ||
| * @return The HttpStream that represents this Request/Response Pair. It can be closed at any time during the | ||
| * request/response, but must be closed by the user thread making this request when it's done. | ||
| */ | ||
| public HttpStreamBase makeRequest(HttpRequestBase request, HttpStreamBaseResponseHandler streamHandler, | ||
| boolean useManualDataWrites) throws CrtRuntimeException { |
There was a problem hiding this comment.
IIRC, we have some restriction on with manual write, the request doesn't allow to have an input stream with it. Is that true?
There was a problem hiding this comment.
Do you mean the body stream? then yes, when manual writes is on, having a body stream with h1 would fail but h2 is supposed to send them one after the other and is handled by the H2 protocol..
There was a problem hiding this comment.
yeah, that's a very inconsistent behavior, either we document it out clearly, or try to just handle it.
And if we decided to error out for the case, I think we should error out early in the binding, so that it's more clear comparing to our C error handling with one error code and need to look deep into the log to figure out the error
| /** | ||
| * {@inheritDoc} | ||
| * <p> | ||
| * <b>HTTP/1.1 restriction:</b> An HTTP/1.1 request message body is framed by | ||
| * exactly one mechanism: either a {@code Content-Length} header declaring the | ||
| * body size upfront, or {@code Transfer-Encoding: chunked} for streaming | ||
| * (RFC 9112, Section 6). A sender MUST NOT combine both framing mechanisms | ||
| * (RFC 9112, Section 6.2: "A sender MUST NOT send a Content-Length header field | ||
| * in any message that contains a Transfer-Encoding header field"). | ||
| * <p> | ||
| * Because the framing is committed at the start of the message, a body stream | ||
| * and manual {@code writeData()} calls cannot coexist on the same HTTP/1.1 | ||
| * stream — doing so would either violate the declared {@code Content-Length} or | ||
| * require switching transfer-encoding mid-message, which the protocol does not | ||
| * permit. If the request was created with an {@link HttpRequestBodyStream}, | ||
| * calling this method will throw {@link IllegalStateException}. | ||
| * <p> | ||
| * HTTP/2 does not have this restriction. HTTP/2 uses its own DATA frame | ||
| * framing (RFC 9113, Section 8.1), where the body stream and manual writes | ||
| * both append DATA frames to the same outgoing queue. | ||
| * <p> | ||
| * <b>Migration from writeChunk:</b> This method supersedes the deprecated | ||
| * {@link #writeChunk} methods. Use {@code writeData} for all manual body data | ||
| * writes on both HTTP/1.1 and HTTP/2 streams. | ||
| */ |
There was a problem hiding this comment.
I think this should be moved to the make_request instead, since it should target on the error case where manual_write is set with a http message with the body, right?
Also, I am not sure I follow the reason why we cannot we have the body stream and manual write.
| * Tests that makeRequest throws IllegalStateException when called with both | ||
| * a body stream and useManualDataWrites=true on an HTTP/1.1 connection. | ||
| */ | ||
| @Test(expected = IllegalStateException.class) |
There was a problem hiding this comment.
I am a bit leaning towards to use try catch the exception from the exact method we invoke, so that it will be assert on the exact exception raised from where we think they should be raised.
from my understanding, this will be assert on the whole test raise the exception?
|
|
||
| return (HttpStream)stream; | ||
| HttpStream h1Stream = (HttpStream)stream; | ||
| h1Stream.setHasBodyStream(request.getBodyStream() != null); |
2 participants
Issue #, if available:
Description of changes:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.