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

Support handshake timeout in websocket handlers #8856

Merged
merged 9 commits into from May 22, 2019

Conversation

Projects
None yet
5 participants
@qeesung
Copy link
Contributor

commented Feb 10, 2019

Motivation:

Support handshake timeout option in websocket handlers. It makes sense to limit the time we need to move from HANDSHAKE_ISSUED to HANDSHAKE_COMPLETE states when upgrading to WebSockets

Modification:

  • Add handshakeTimeoutMillis option in WebSocketClientProtocolHandshakeHandler and WebSocketServerProtocolHandshakeHandler.
  • Schedule a timeout task, the task will trigger user event HANDSHAKE_TIMEOUT if the handshake timed out.

Result:

Fixes issue #8841

@netty-bot

This comment has been minimized.

Copy link

commented Feb 10, 2019

Can one of the admins verify this patch?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.35.Final milestone Mar 8, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 25, 2019

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@trustin can you have a look as well ?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@kachayev PTAL as well

@normanmaurer

This comment has been minimized.

Copy link
Member

commented Apr 10, 2019

@qeesung please rebase

Show resolved Hide resolved ...ndler/codec/http/websocketx/WebSocketClientProtocolHandshakeHandler.java Outdated
Show resolved Hide resolved ...ndler/codec/http/websocketx/WebSocketClientProtocolHandshakeHandler.java Outdated
public void applyHandshakeTimeout() {
final ChannelPromise localHandshakePromise = handshakePromise;
final long handshakeTimeoutMillis = this.handshakeTimeoutMillis;
if (handshakeTimeoutMillis <= 0 || localHandshakePromise.isDone()) {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 10, 2019

Member

do we also need to check if localHandshakePromise == null ?

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 10, 2019

Author Contributor

handshakePromise is initialized when the handle is added. Is there a case where handshakePromise is empty?

});
}

public ChannelFuture getHandshakeFuture() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 10, 2019

Member

seems like this method is not used at all ?

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 10, 2019

Author Contributor

getHandshakeFuture method will be used in test cases, and I've set it to package-private.

Show resolved Hide resolved ...ndler/codec/http/websocketx/WebSocketServerProtocolHandshakeHandler.java Outdated
Show resolved Hide resolved ...ndler/codec/http/websocketx/WebSocketServerProtocolHandshakeHandler.java Outdated
Show resolved Hide resolved ...ndler/codec/http/websocketx/WebSocketServerProtocolHandshakeHandler.java Outdated
Show resolved Hide resolved .../netty/handler/codec/http/websocketx/WebSocketHandshakeHandOverTest.java Outdated
Show resolved Hide resolved .../netty/handler/codec/http/websocketx/WebSocketHandshakeHandOverTest.java Outdated
Show resolved Hide resolved .../netty/handler/codec/http/websocketx/WebSocketHandshakeHandOverTest.java Outdated

@qeesung qeesung force-pushed the qeesung:websocket_handshake_timeout branch from 3dcd911 to 0c1619d Apr 10, 2019

@@ -200,12 +206,18 @@ public void handlerAdded(ChannelHandlerContext ctx) {
if (cp.get(WebSocketClientProtocolHandshakeHandler.class) == null) {
// Add the WebSocketClientProtocolHandshakeHandler before this one.
ctx.pipeline().addBefore(ctx.name(), WebSocketClientProtocolHandshakeHandler.class.getName(),
new WebSocketClientProtocolHandshakeHandler(handshaker));
new WebSocketClientProtocolHandshakeHandler(handshaker)
.handshakeTimeoutMillis(handshakeTimeoutMillis));

This comment has been minimized.

Copy link
@trustin

trustin Apr 15, 2019

Member

Can we just pass this in via an additional constructor parameter? No need to introduce a mutable state.

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 24, 2019

Author Contributor

Done!

@@ -154,7 +160,8 @@ public void handlerAdded(ChannelHandlerContext ctx) {
// Add the WebSocketHandshakeHandler before this one.
ctx.pipeline().addBefore(ctx.name(), WebSocketServerProtocolHandshakeHandler.class.getName(),
new WebSocketServerProtocolHandshakeHandler(websocketPath, subprotocols,
allowExtensions, maxFramePayloadLength, allowMaskMismatch, checkStartsWith));
allowExtensions, maxFramePayloadLength, allowMaskMismatch, checkStartsWith)
.handshakeTimeoutMillis(handshakeTimeoutMillis));

This comment has been minimized.

Copy link
@trustin

trustin Apr 15, 2019

Member

Ditto - no need to have a mutable state.

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 24, 2019

Author Contributor

Done!

@@ -38,9 +38,11 @@
* {@link ClientHandshakeStateEvent#HANDSHAKE_ISSUED} or {@link ClientHandshakeStateEvent#HANDSHAKE_COMPLETE}.
*/
public class WebSocketClientProtocolHandler extends WebSocketProtocolHandler {
private static final long DEFAULT_HANDSHAKE_TIMEOUT = 10000L;

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Apr 25, 2019

Member

DEFAULT_HANDSHAKE_TIMEOUT_MS ?

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 25, 2019

Author Contributor

DONE

super(dropPongFrames);
this.handshaker = handshaker;
this.handleCloseFrames = handleCloseFrames;
this.handshakeTimeoutMillis = handshakeTimeoutMillis;

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Apr 25, 2019

Member

check positive?

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 25, 2019

Author Contributor

Done


private final String websocketPath;
private final String subprotocols;
private final boolean allowExtensions;
private final int maxFramePayloadSize;
private final boolean allowMaskMismatch;
private final boolean checkStartsWith;
private final long handshakeTimeoutMillis;
private volatile ChannelHandlerContext ctx;

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Apr 25, 2019

Member

why is this volatile?

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 25, 2019

Author Contributor

You're right. There's no need for volatile.


private final String websocketPath;
private final String subprotocols;
private final boolean allowExtensions;
private final int maxFramePayloadSize;
private final boolean allowMaskMismatch;
private final boolean checkStartsWith;
private final long handshakeTimeoutMillis;
private volatile ChannelHandlerContext ctx;
private volatile ChannelPromise handshakePromise;

This comment has been minimized.

Copy link
@carl-mastrangelo

carl-mastrangelo Apr 25, 2019

Member

Also, why is this volatile?

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 25, 2019

Author Contributor

Done

@carl-mastrangelo
Copy link
Member

left a comment

LGTM. I'll defer to either @normanmaurer or @trustin to review / merge.

return;
}

final ScheduledFuture<?> timeoutFuture = ctx.executor().schedule(new Runnable() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 29, 2019

Member

nit: this could be Future<?>

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 29, 2019

Author Contributor

Done!

return;
}

final ScheduledFuture<?> timeoutFuture = ctx.executor().schedule(new Runnable() {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Apr 29, 2019

Member

nit: could be Future<?>

This comment has been minimized.

Copy link
@qeesung

qeesung Apr 29, 2019

Author Contributor

Done!

@normanmaurer normanmaurer requested a review from trustin Apr 29, 2019

final Future<?> timeoutFuture = ctx.executor().schedule(new Runnable() {
@Override
public void run() {
if (localHandshakePromise.isDone()) {

This comment has been minimized.

Copy link
@trustin

trustin May 10, 2019

Member

This is redundant because it's already guarded by tryFailure below.

This comment has been minimized.

Copy link
@qeesung

qeesung May 10, 2019

Author Contributor

Done!

@qeesung

This comment has been minimized.

Copy link
Contributor Author

commented May 10, 2019

@trustin Fixed! Would you please review it again? :-)

@normanmaurer normanmaurer merged commit 5ffac03 into netty:4.1 May 22, 2019

@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 22, 2019

@qeesung thanks a lot!

normanmaurer added a commit that referenced this pull request May 22, 2019

Support handshake timeout in websocket handlers (#8856)
Motivation:

Support handshake timeout option in websocket handlers. It makes sense to limit the time we need to move from `HANDSHAKE_ISSUED` to `HANDSHAKE_COMPLETE` states when upgrading to WebSockets

Modification:

- Add `handshakeTimeoutMillis` option in `WebSocketClientProtocolHandshakeHandler`  and `WebSocketServerProtocolHandshakeHandler`.
- Schedule a timeout task, the task will trigger user event `HANDSHAKE_TIMEOUT` if the handshake timed out.

Result:

Fixes issue #8841

@qeesung qeesung deleted the qeesung:websocket_handshake_timeout branch May 24, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.