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

Issue #5150 - Infinite connection timeout support in ManagedSelector #5151

Merged
merged 2 commits into from Aug 14, 2020

Conversation

olegmoz
Copy link
Contributor

@olegmoz olegmoz commented Aug 13, 2020

Closes #5150
ManagedSelector never timeouts connection in case connection timeout is set to zero.

@joakime
Copy link
Contributor

joakime commented Aug 13, 2020

Thank you for the pull request.

But we need to satisfy the requirements of Eclipse Legal in order to accept / merge this pull request.

The Eclipse Foundation ECA validation has failed for the commits on this pull request.

See the "Details" link on the eclipsefdn/eca check to know what you have failed to do.

See also https://github.com/eclipse/jetty.project/blob/jetty-10.0.x/CONTRIBUTING.md#eclipse-contributor-agreement

Signed-off-by: Oleg Mozzhechkov <oleg.mozzhechkov@gmail.com>
@olegmoz olegmoz force-pushed the 5150-zero-connection-timeout branch from e4813e4 to bffd6a2 Compare August 13, 2020 14:36
@olegmoz
Copy link
Contributor Author

olegmoz commented Aug 13, 2020

@joakime thanks, I have signed-off the commit

Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

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

LGTM.

@sbordet you should review this too.

@joakime joakime requested a review from sbordet August 13, 2020 14:46
@joakime joakime added this to In progress in Jetty 9.4.32 via automation Aug 13, 2020
@joakime joakime moved this from In progress to Review in progress in Jetty 9.4.32 Aug 13, 2020
@joakime joakime changed the title #5150 - Infinite connection timeout support in ManagedSelector Issue #5150 - Infinite connection timeout support in ManagedSelector Aug 13, 2020
Copy link
Contributor

@sbordet sbordet left a comment

Choose a reason for hiding this comment

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

Can you please update the javadocs for HttpClient.setConnectionTimeout() and HttpClient.getConnectTimeout(), adding that the value 0 means infinite timeout?
Other than that looks good.

@@ -888,7 +888,11 @@ public String toString()
{
this.channel = channel;
this.attachment = attachment;
this.timeout = ManagedSelector.this._selectorManager.getScheduler().schedule(this, ManagedSelector.this._selectorManager.getConnectTimeout(), TimeUnit.MILLISECONDS);
final long timeout = ManagedSelector.this._selectorManager.getConnectTimeout();
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove final, not necessary.

Signed-off-by: Oleg Mozzhechkov <oleg.mozzhechkov@gmail.com>
@olegmoz
Copy link
Contributor Author

olegmoz commented Aug 13, 2020

@sbordet thanks, removed final and updated the javadocs. Please review

@olegmoz olegmoz requested a review from sbordet August 13, 2020 18:26
Jetty 9.4.32 automation moved this from Review in progress to Reviewer approved Aug 14, 2020
@sbordet sbordet merged commit a6e1f9d into jetty:jetty-9.4.x Aug 14, 2020
Jetty 9.4.32 automation moved this from Reviewer approved to Done Aug 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Jetty 9.4.32
  
Done
Development

Successfully merging this pull request may close these issues.

Zero connection timeout is not supported in HTTP client with non-blocking connect
3 participants