WebSocket client handshaker to support "force close" after timeout by kachayev · Pull Request #8896 · netty/netty

kachayev · 2019-02-27T22:09:27Z

Motivation:

RFC 6455 defines that, generally, a WebSocket client should not close a TCP
connection as far as a server is the one who's responsible for doing that.
In practice tho', it's not always possible to control the server. Server's
misbehavior may lead to connections being leaked (if the server does not
comply with the RFC).

RFC 6455 #7.1.1 says

In abnormal cases (such as not having received a TCP Close from the server
after a reasonable amount of time) a client MAY initiate the TCP Close.

Modifications:

WebSocket client handshaker additional param forceCloseAfterMillis
Switched off by default to keep backward compatibility

Result:

WebSocket client handshaker to comply with RFC. Fixes #8883.

Open questions/comments:

I've implemented this as a WebSocketClientHandshaker functionality, not in handlers, as far as it's responsible for close.
Nothing prevents you to call handshaker.close twice, in such a case only first timeout would be applied.
Do we need to keep this feature switched OFF by default to ensure backward compatibility? I mean... it's hard to justify current implementation and I think 10 seconds default timeout should be enough to cover potential drawbacks. @normanmaurer WDYT?
Testing is kind of clumsy... It's impossible to detect if the channel was closed because of timeout or because of the server or... whatever. Maybe I'm missing any better approach to test this?

netty-bot · 2019-02-27T22:12:04Z

Can one of the admins verify this patch?

normanmaurer · 2019-02-28T07:44:57Z

@kachayev its RFC6455 ;)

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

...ttp/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketHandshakeHandOverTest.java

kachayev · 2019-02-28T09:39:00Z

@kachayev its RFC6455 ;)

@normanmaurer Copy-paste error ;)

kachayev · 2019-02-28T13:29:22Z

@normanmaurer Updated. I still think if need to cover this use case:

handshaker.setForceCloseTimeoutMillis(1_000).close(ch, new CloseWebsocketFrame());

It might be useful if the timeout depends on what happened during the communication (then I have no chance to setup it properly before the upgrade)... I just don't know how often you might need this in practice.

normanmaurer · 2019-02-28T13:35:33Z

@kachayev another alternative would be to allow to overload the method with a timeout.

kachayev · 2019-02-28T14:16:01Z

@normanmaurer Added setter to cover more use cases. Changed test to use an internal flag that indicated that the timeout was actually fired.

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

...ttp/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketHandshakeHandOverTest.java

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker13.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker08.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker07.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java

normanmaurer · 2019-03-07T09:45:16Z

/cc @vietj

normanmaurer · 2019-03-08T19:32:23Z

@netty-bot test this please

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

normanmaurer · 2019-03-15T11:18:38Z

@netty-bot test this please

normanmaurer

LGTM... just one nit

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

normanmaurer · 2019-03-15T20:17:28Z

@netty-bot test this please

normanmaurer · 2019-03-26T08:20:42Z

@trustin can you have a look as well please ?

normanmaurer · 2019-03-26T08:21:44Z

@netty-bot test this please

trustin · 2019-04-01T10:03:45Z

Switched off by default to keep backward compatibility

Probably better enabling by default with reasonable default? Leaky connection for backward compatibility doesn't sound right to me.

normanmaurer · 2019-04-01T10:05:57Z

@trustin sounds fine to me... maybe 3 seconds (which is also the default we use for SSL close notify) is ok ? Or do we want to use 10 seconds as we use for handshake timeout in SSL by default ?

trustin

Mostly nits.

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

...c-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker00.java

trustin · 2019-04-01T10:09:50Z

Not a WebSocket expert. 😅 Leaving it up to @kachayev 😆

normanmaurer · 2019-04-01T10:10:39Z

@slandelle can you have a look as well ?

kachayev · 2019-04-01T16:51:58Z

@trustin

enabling by default

Absolutely, 100% for it. I will put 10 seconds to stay consistent with SSL handshake timeouts.

slandelle

2 small nits. LGTM otherwise!

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java

Motivation: RFC 6455 defines that, generally, a WebSocket client should not close a TCP connection as far as a server is the one who's responsible for doing that. In practice tho', it's not always possible to control the server. Server's misbehavior may lead to connections being leaked (if the server does not comply with the RFC). RFC 6455 netty#7.1.1 says > In abnormal cases (such as not having received a TCP Close from the server after a reasonable amount of time) a client MAY initiate the TCP Close. Modifications: * WebSocket client handshaker additional param `forceCloseAfterMillis` * Switched of by default to keep backward compatibility Result: WebSocket client handshaker to comply with RFC. Fixes netty#8883.

kachayev · 2019-04-02T05:32:29Z

@trustin @slandelle Pushed changes, thanks for the review!

normanmaurer · 2019-04-02T09:00:46Z

@trustin PTAL again

normanmaurer · 2019-04-02T09:00:56Z

@netty-bot test this please

normanmaurer · 2019-04-10T13:25:42Z

@kachayev thanks a lot!

…8896) Motivation: RFC 6455 defines that, generally, a WebSocket client should not close a TCP connection as far as a server is the one who's responsible for doing that. In practice tho', it's not always possible to control the server. Server's misbehavior may lead to connections being leaked (if the server does not comply with the RFC). RFC 6455 #7.1.1 says > In abnormal cases (such as not having received a TCP Close from the server after a reasonable amount of time) a client MAY initiate the TCP Close. Modifications: * WebSocket client handshaker additional param `forceCloseAfterMillis` * Use 10 seconds as default Result: WebSocket client handshaker to comply with RFC. Fixes #8883.

…etty#8896) Motivation: RFC 6455 defines that, generally, a WebSocket client should not close a TCP connection as far as a server is the one who's responsible for doing that. In practice tho', it's not always possible to control the server. Server's misbehavior may lead to connections being leaked (if the server does not comply with the RFC). RFC 6455 #7.1.1 says In abnormal cases (such as not having received a TCP Close from the server after a reasonable amount of time) a client MAY initiate the TCP Close. Modifications: WebSocket client handshaker additional param forceCloseAfterMillis Switched off by default to keep backward compatibility Result: WebSocket client handshaker to comply with RFC. Fixes (netty#8883).

normanmaurer requested changes Feb 28, 2019

View reviewed changes

kachayev force-pushed the ft-websocket-force-close branch 2 times, most recently from 326620a to 1e91a34 Compare February 28, 2019 13:21

kachayev force-pushed the ft-websocket-force-close branch from 1e91a34 to c150a74 Compare February 28, 2019 14:14

normanmaurer requested changes Feb 28, 2019

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

...ttp/src/test/java/io/netty/handler/codec/http/websocketx/WebSocketHandshakeHandOverTest.java Outdated Show resolved Hide resolved

normanmaurer requested a review from slandelle February 28, 2019 20:11

kachayev force-pushed the ft-websocket-force-close branch 2 times, most recently from abac67d to cd8b306 Compare February 28, 2019 20:17

normanmaurer requested changes Mar 1, 2019

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

kachayev force-pushed the ft-websocket-force-close branch from cd8b306 to be40c4e Compare March 1, 2019 08:27

normanmaurer reviewed Mar 1, 2019

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

kachayev force-pushed the ft-websocket-force-close branch from be40c4e to f88cc9b Compare March 1, 2019 21:49

normanmaurer requested changes Mar 5, 2019

View reviewed changes

kachayev force-pushed the ft-websocket-force-close branch from f88cc9b to 481d380 Compare March 5, 2019 09:30

normanmaurer approved these changes Mar 7, 2019

View reviewed changes

normanmaurer requested review from carl-mastrangelo and ejona86 March 7, 2019 09:45

normanmaurer modified the milestones: 4.1.34.Final, 4.1.35.Final Mar 7, 2019

normanmaurer requested changes Mar 14, 2019

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

kachayev force-pushed the ft-websocket-force-close branch from 481d380 to a3407f5 Compare March 14, 2019 21:41

normanmaurer requested changes Mar 15, 2019

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

kachayev force-pushed the ft-websocket-force-close branch from a3407f5 to d770d0b Compare March 15, 2019 14:13

normanmaurer approved these changes Mar 15, 2019

View reviewed changes

normanmaurer requested a review from trustin March 26, 2019 08:20

trustin reviewed Apr 1, 2019

View reviewed changes

slandelle requested changes Apr 1, 2019

View reviewed changes

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

codec-http/src/main/java/io/netty/handler/codec/http/websocketx/WebSocketClientHandshaker.java Outdated Show resolved Hide resolved

kachayev force-pushed the ft-websocket-force-close branch from d770d0b to d6795c4 Compare April 2, 2019 05:30

normanmaurer requested review from slandelle and trustin April 2, 2019 06:42

slandelle approved these changes Apr 2, 2019

View reviewed changes

normanmaurer self-assigned this Apr 10, 2019

normanmaurer merged commit ee351ef into netty:4.1 Apr 10, 2019

kachayev mentioned this pull request Apr 22, 2019

Upgrade Netty to 4.1.36.Final clj-commons/aleph#507

Merged

Uh oh!

Conversation

kachayev commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

netty-bot commented Feb 27, 2019

Uh oh!

normanmaurer commented Feb 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kachayev commented Feb 28, 2019

Uh oh!

kachayev commented Feb 28, 2019

Uh oh!

normanmaurer commented Feb 28, 2019

Uh oh!

kachayev commented Feb 28, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

normanmaurer commented Mar 7, 2019

Uh oh!

normanmaurer commented Mar 8, 2019

Uh oh!

Uh oh!

normanmaurer commented Mar 15, 2019

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

normanmaurer commented Mar 15, 2019

Uh oh!

normanmaurer commented Mar 26, 2019

Uh oh!

normanmaurer commented Mar 26, 2019

Uh oh!

trustin commented Apr 1, 2019

Uh oh!

normanmaurer commented Apr 1, 2019

Uh oh!

trustin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

trustin commented Apr 1, 2019

Uh oh!

normanmaurer commented Apr 1, 2019

Uh oh!

kachayev commented Apr 1, 2019

Uh oh!

slandelle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kachayev commented Apr 2, 2019

Uh oh!

normanmaurer commented Apr 2, 2019

Uh oh!

normanmaurer commented Apr 2, 2019

Uh oh!

normanmaurer commented Apr 10, 2019

kachayev commented Feb 27, 2019 •

edited

Loading