-
Notifications
You must be signed in to change notification settings - Fork 910
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
Correctly throw WriteTimeoutException
for requests without content
#4259
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4259 +/- ##
============================================
- Coverage 73.38% 73.37% -0.02%
+ Complexity 16985 16982 -3
============================================
Files 1451 1451
Lines 64034 64033 -1
Branches 8039 8039
============================================
- Hits 46994 46983 -11
- Misses 12952 12953 +1
- Partials 4088 4097 +9
Continue to review full report at Codecov.
|
WriteTimeoutException
for requests without contentWriteTimeoutException
for requests without content
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.
Thanks for the quick fix. 🙇♂️
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.
Thanks for fixing this. I wish we can add a test for it soon.
Motivation: - Currently, there is no easy way to create integration tests which simulate certain behaviors (e.g. `WriteTimeoutException`) - #4259 - Users may also want to customize behavior like throttling, fault injection, etc. for clients. Modifications: - Add `CHANNEL_PIPELINE_CUSTOMIZER` to `ClientFactoryOptions`, which is a consumer on `ChannelPipeline` - I'm a little skeptical of adding these methods to `ClientFactoryBuilder`, `Flags` until more users want this feature. - Call `HttpClientFactory#channelPipelineCustomizer` right before `channel#connect`, giving users to customize the pipeline at the last minute. - Add a unit test which simulates write timeout behavior. - Rename `invokeProxyConnectFailed` to `maybeHandleProxyFailure` to better represent what the method is doing. Result: - Closes #3907
Motivation:
WriteTimeoutException
is defined as an exception thrown if the first message write doesn't complete within a specified time. This specification holds for requests with bodies.However, requests without bodies never throw a
WriteTimeoutException
because:armeria/core/src/main/java/com/linecorp/armeria/internal/common/stream/FixedStreamMessage.java
Lines 141 to 148 in 5a2c720
armeria/core/src/main/java/com/linecorp/armeria/client/HttpRequestSubscriber.java
Line 261 in 5a2c720
Modifications:
HttpRequestSubscriber#cancelTimeout
when the subscription on the request is complete.Result:
WriteTimeoutException
may not be thrown for header-only requests #4255Verified the following test passes:
https://github.com/jrhee17/armeria/blob/13c2c60e60b6109531b363d55d09ab6af7de432c/core/src/test/java/com/linecorp/armeria/client/TimeoutTest.java
Also created a PR which helps simulate slow writes as a follow-up:
#4260