Skip to content
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

feat: java11 request options timeout. #1233

Merged
merged 4 commits into from Jul 2, 2020
Merged

feat: java11 request options timeout. #1233

merged 4 commits into from Jul 2, 2020

Conversation

ChunMengLu
Copy link
Contributor

Config feign-java11 request timeout use feign Request.options.

Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change looks good, but, can we have some testing as well?

@velo velo added feedback provided Feedback has been provided to the author waiting for votes Enhancements or changes proposed that need more support before consideration labels Jun 2, 2020
@ChunMengLu
Copy link
Contributor Author

ChunMengLu commented Jun 2, 2020

java11 javadoc:

HttpRequest.Builder timeout​(Duration duration)
Sets a timeout for this request. If the response is not received within the specified timeout then an HttpTimeoutException is thrown from HttpClient::send or HttpClient::sendAsync completes exceptionally with an HttpTimeoutException. The effect of not setting a timeout is the same as setting an infinite Duration, ie. block forever.
Parameters:
duration - the timeout duration
Returns:
this builder
Throws:
IllegalArgumentException - if the duration is non-positive

But I found that it is not very good in the actual test, and running the test directly does not work, but I broke the point to the jdk.internal.net.http.HttpClientImpl#purgeTimeoutsAndReturnNextDeadline method, and found that timeout is actually supported,The jdk.internal.net.http.ResponseTimerEvent#handle method will execute and throw an exception.

Windows 10 and amazon-corretto-11.0.7.10.1-windows-x64.

Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, can merge once the build is passing.

@velo velo added needs info Information is either missing or incomplete. and removed waiting for votes Enhancements or changes proposed that need more support before consideration labels Jun 9, 2020
@ChunMengLu
Copy link
Contributor Author

LGTM, can merge once the build is passing.

ok!

@kdavisk6 kdavisk6 added ready to merge Will be merged if no other member ask for changes and removed needs info Information is either missing or incomplete. labels Jun 17, 2020
@kdavisk6 kdavisk6 merged commit ae514aa into OpenFeign:master Jul 2, 2020
@gb-klein
Copy link
Contributor

gb-klein commented Nov 9, 2020

@ChunMengLu , @velo : One thing i noticed is, that you only considered the request timeout, but not the connect timeout. This would have to be set on the HttpClient instance (e.g. via Builder::connectTimeout). One could do this manually by providing a custom built HttpClient to Http2Client. However it would be good to leave this over to Http2Client when it constructs the HttpClient for us. E.g. by replacing the default constructor with a constructor taking Options or (IMHO better) lazily instantiating HttpClient - if no custom was specified via constructor - when the request should be sent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided Feedback has been provided to the author ready to merge Will be merged if no other member ask for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants