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
Configurable to disable streaming mode for Default client #1182
Conversation
* Add a property `allowStramingMode` in `Request.Option`. * Optional to disable the streaming mode for `HttpURLConnection`. * The property is default to `true` for compatibility. Fix OpenFeign#260
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.
added a few comments to get the conversation started
if (contentLength != null) { | ||
connection.setFixedLengthStreamingMode(contentLength); | ||
} else { | ||
connection.setChunkedStreamingMode(8196); |
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.
maybe make this configurable?!
@@ -269,6 +291,7 @@ public Options(long connectTimeout, TimeUnit connectTimeoutUnit, | |||
this.readTimeout = readTimeout; | |||
this.readTimeoutUnit = readTimeoutUnit; | |||
this.followRedirects = followRedirects; | |||
this.allowStreamingMode = true; |
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.
like the idea of defaulting to true
core/src/main/java/feign/Client.java
Outdated
connection.setFixedLengthStreamingMode(contentLength); | ||
} else { | ||
connection.setChunkedStreamingMode(8196); | ||
if (options.isAllowStreamingMode()) { |
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.
From the documentation, the 2 set*StreamingMode()
states:
This method is used to enable streaming of a HTTP request body without internal buffering, when the content length
what if we call this new option disableInternalBuffering
?
allowStreamingMode
makes me scratch the head.
Also, this seem to be something very specific to java.net.HttpURLConnection
. May be move to Client.Default
?
or is this something we can enable for ok_http, hc5, apache http 4, google http and so on.
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.
Yes, indeed, it's a feature for java.net.HttpURLConnection
, moving the option to Client.Default
is better. Why didn't I think of that? :-D
I call it allowStreamingMode
because the word is just literal. I think if one knows the 2 set*StreamingMode()
methods in HttpURLConnection
, so he can understand what streaming mode is.
After moving allowStreamingMode
into Client.Default
, it will be related to that specific implementation. Then it may be easier to understand.
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.
yeah, in my mind, allowStreamingMode
doesn't say much, like allowFlushingMode
... I have no idea what either mean.
now, disableInternalBuffering
, at least for me, has an exact meaning... client has some kind of internal buffer, and it's being turned off.
Anyway, I won't be picky about this, do what you feel best about.
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.
Discussed with my colleagues. Now we all agree allowStreamingMode
is not so good, and we checked the source code found the buffering only happens in writing request output stream, so the final desicion is call it disableRequestBuffering
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.
Sound great
* Move it from `Request.Options` to `Default` client
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.
Lgtm
You need to run |
Thanks for the tip. I find a funny thing. The I know it's not very well to do such thing, but the |
* Update the license description * Ignore the new test for google http client
Add a property
allowStramingMode
inRequest.Option
.Optional to disable the streaming mode for
HttpURLConnection
.The property is default to
true
for compatibility.Fix #260