-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix #326: Requests write to output streams, they don't create inputstreams #331
Conversation
045918d
to
8b661a7
Compare
request.body().get().writeTo(bytes); | ||
} catch (IOException e) { | ||
throw new SafeRuntimeException("Failed to create a BodyPublisher", e); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same block from ConjureBodySerDe, but it's only required for HttpChannel, not socket based transports.
} catch (IOException e) { | ||
throw new SafeRuntimeException("Failed to create a BodyPublisher", e); | ||
} | ||
return HttpRequest.BodyPublishers.ofByteArray(bytes.toByteArray()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we're able to use the more efficient ofByteArray publisher rather than ofInputStream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that before, octet-stream requests would throw (not-implemented) where here they're fully buffered on heap like our feign client. This can be dangerous, but I don't think we should spend the time to optimize that case unless we decide to go all in on the jdk11 client.
@@ -100,8 +99,8 @@ public RequestBody serialize(BinaryRequestBody value) { | |||
} | |||
|
|||
@Override | |||
public InputStream content() { | |||
throw new UnsupportedOperationException("TODO(rfink): implement this"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: implement tests for binary requests in the abstract channel test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels weird that we're requiring a request body to advertise its size but also providing an API which encourages the implementer to omit it. I worry that avoiding the duplication of bytes might be pre-mature optimization and we should sit on this until we have some better benchmarks or real life usage
I'd be in favor of removing request body size entirely. |
I think this both simplifies the API, ignoring the performance benefit entirely. |
Would you intend to omit the header entirely? It seems we'd be putting ourself in a bind to try and populate the header from within the channel if we decided to not buffer binary requests.
I agree that it does simplify the API but I would still prefer to focus on getting the current client out before making too many changes |
The http client libraries I'm aware of tend to either take chunks of data from a callback/consumer (async-io) or provide an OutputStream to write data to. I don't think we should invent a new model for our API that makes it more difficult and less performant to adapt to clients unless we have a strong reason for it.
The Content-Length header? Many clients will buffer up to N bytes (often 8 or 16k) and send content-length if the data is fully buffered, otherwise send chunked requests. I think that's a reasonable approach for us to take.
There's not really a downside to sending chunked requests, especially compared to buffering. Binary request buffering gets a little bit funky, but I think it's the same problem that we have a TODO to implement already, that's only going to be special for async-io clients (e.g. netty and jdk9 client) where InputStream has similar problems, the jdk9 input stream body producer is complicated and we're better off avoiding it if we can -- this will give us a better idea of the real performance of the client.
Fully supportive of getting what we have into a good state but I'm also worried that we'll gather metrics to pick our target client using data from a subpar implementation. Once we start to roll dialogue out I imagine API changes like this will be harder to make. |
👍 ok, lets move ahead with this |
b6c3bd3
to
08715b4
Compare
08715b4
to
80953f5
Compare
Generate changelog in
|
@@ -67,13 +65,8 @@ | |||
|
|||
private final RequestBody body = new RequestBody() { | |||
@Override | |||
public OptionalLong length() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when we know the length we probably want to write the Content-Length header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clients which buffer requests prior to sending them do send a content-length. This change allowed us to avoid fully buffering requests in many cases as we write directly to a socket.
HttpChannel (java 9 client) could set a content-length header, though it never did before.
See #326 for details.