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

SslHandler.setHandshakeTimeout*(...) should also been enforced on the… #7277

Closed
wants to merge 1 commit into
base: 4.1
from

Conversation

@normanmaurer
Member

normanmaurer commented Oct 5, 2017

… server side.

Motivation:

We should also enforce the handshake timeout on the server-side to allow closing connections which will not finish the handshake in an expected amount of time.

Modifications:

  • Enforce the timeout on the server and client side
  • Add unit test.

Result:

Fixes [#7230].

EmbeddedChannel ch = new EmbeddedChannel(handler);
try {
while (!handler.handshakeFuture().isDone()) {
Thread.sleep(10);

This comment has been minimized.

@netkins

netkins Oct 5, 2017

CRITICAL Remove this use of "Thread.sleep()". rule

@netkins

netkins Oct 5, 2017

CRITICAL Remove this use of "Thread.sleep()". rule

This comment has been minimized.

@normanmaurer

normanmaurer Oct 5, 2017

Member

This is needed...

@normanmaurer

normanmaurer Oct 5, 2017

Member

This is needed...

This comment has been minimized.

@ejona86

ejona86 Oct 23, 2017

Contributor

Swapping from isDone() to await(10, MILLISECONDS) is probably better.

@ejona86

ejona86 Oct 23, 2017

Contributor

Swapping from isDone() to await(10, MILLISECONDS) is probably better.

This comment has been minimized.

@normanmaurer

normanmaurer Oct 23, 2017

Member

@ejona86 actually this will not work as then we will try to call await() on the EventLoop thread which will then throw an exception.

@normanmaurer

normanmaurer Oct 23, 2017

Member

@ejona86 actually this will not work as then we will try to call await() on the EventLoop thread which will then throw an exception.

SslHandler.setHandshakeTimeout*(...) should also been enforced on the…
… server side.

Motivation:

We should also enforce the handshake timeout on the server-side to allow closing connections which will not finish the handshake in an expected amount of time.

Modifications:

- Enforce the timeout on the server and client side
- Add unit test.

Result:

Fixes [#7230].
@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Oct 6, 2017

Contributor

@normanmaurer looks like the default handshake timeout is 10 seconds which will now be applied to server mode?

Contributor

johnou commented Oct 6, 2017

@normanmaurer looks like the default handshake timeout is 10 seconds which will now be applied to server mode?

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 6, 2017

Member

@johnou yep... do you think this is too aggressive ?

Member

normanmaurer commented Oct 6, 2017

@johnou yep... do you think this is too aggressive ?

@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Oct 6, 2017

Contributor

@normanmaurer unfortunately I don't have any metrics for handshakes.. I will discuss it with my team and get back to you.

Contributor

johnou commented Oct 6, 2017

@normanmaurer unfortunately I don't have any metrics for handshakes.. I will discuss it with my team and get back to you.

@johnou

This comment has been minimized.

Show comment
Hide comment
@johnou

johnou Oct 6, 2017

Contributor

@normanmaurer we are fine with the 10s timeout, heads up @rkapsi

Contributor

johnou commented Oct 6, 2017

@normanmaurer we are fine with the 10s timeout, heads up @rkapsi

@rkapsi

This comment has been minimized.

Show comment
Hide comment
@rkapsi

rkapsi Oct 6, 2017

Contributor

@johnou @normanmaurer 10s timeout for default is fine for us (and happens to be our configured value). Thx for the heads up @johnou.

Contributor

rkapsi commented Oct 6, 2017

@johnou @normanmaurer 10s timeout for default is fine for us (and happens to be our configured value). Thx for the heads up @johnou.

@rkapsi

This comment has been minimized.

Show comment
Hide comment
@rkapsi

rkapsi Oct 6, 2017

Contributor

Maybe useful in this context... We observe the p95 time for successful handshakes around 1-1.5 seconds and p99 around 3-5 seconds.

Contributor

rkapsi commented Oct 6, 2017

Maybe useful in this context... We observe the p95 time for successful handshakes around 1-1.5 seconds and p99 around 3-5 seconds.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 21, 2017

Member

@ejona86 @louiscryan any concerns with this change?

Member

normanmaurer commented Oct 21, 2017

@ejona86 @louiscryan any concerns with this change?

@ejona86

This comment has been minimized.

Show comment
Hide comment
@ejona86

ejona86 Oct 23, 2017

Contributor

Seems fine to me.

Contributor

ejona86 commented Oct 23, 2017

Seems fine to me.

@normanmaurer

This comment has been minimized.

Show comment
Hide comment
@normanmaurer

normanmaurer Oct 23, 2017

Member

Cherry-picked into 4.1 (521e879) and 4.0 (aa3b5c0)

Member

normanmaurer commented Oct 23, 2017

Cherry-picked into 4.1 (521e879) and 4.0 (aa3b5c0)

@normanmaurer normanmaurer deleted the ssl_timeout_server branch Oct 23, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment