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
velo
left a comment
velo
left a comment
There was a problem hiding this comment.
added a few comments to get the conversation started
| if (contentLength != null) { | ||
| connection.setFixedLengthStreamingMode(contentLength); | ||
| } else { | ||
| connection.setChunkedStreamingMode(8196); |
| this.readTimeout = readTimeout; | ||
| this.readTimeoutUnit = readTimeoutUnit; | ||
| this.followRedirects = followRedirects; | ||
| this.allowStreamingMode = true; |
There was a problem hiding this comment.
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.
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.
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.
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.
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
* Move it from `Request.Options` to `Default` client
velo
added
enhancement
|
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
* Configurable to disable streaming mode for Default client * Add a property `allowStramingMode` in `Request.Option`. * Optional to disable the streaming mode for `HttpURLConnection`. * The property is default to `true` for compatibility. Fix #260 * Fix a merging problem * Make the new option `disableRequestBuffering` to Default client * Move it from `Request.Options` to `Default` client
* Configurable to disable streaming mode for Default client * Add a property `allowStramingMode` in `Request.Option`. * Optional to disable the streaming mode for `HttpURLConnection`. * The property is default to `true` for compatibility. Fix #260 * Fix a merging problem * Make the new option `disableRequestBuffering` to Default client * Move it from `Request.Options` to `Default` client
2 participants
Add a property
allowStramingModeinRequest.Option.Optional to disable the streaming mode for
HttpURLConnection.The property is default to
truefor compatibility.Fix #260