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

WebSocket client handshaker to support "force close" after timeout #8896

Merged
merged 1 commit into from Apr 10, 2019

Conversation

@kachayev
Copy link
Contributor

kachayev commented Feb 27, 2019

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

This comment has been minimized.

Copy link

netty-bot commented Feb 27, 2019

Can one of the admins verify this patch?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 28, 2019

@kachayev its RFC6455 ;)

@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Feb 28, 2019

@kachayev its RFC6455 ;)

@normanmaurer Copy-paste error ;)

@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch 2 times, most recently from 326620a to 1e91a34 Feb 28, 2019
@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Feb 28, 2019

@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

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 28, 2019

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

@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from 1e91a34 to c150a74 Feb 28, 2019
@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Feb 28, 2019

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

@normanmaurer normanmaurer requested a review from slandelle Feb 28, 2019
@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch 2 times, most recently from abac67d to cd8b306 Feb 28, 2019
@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from cd8b306 to be40c4e Mar 1, 2019
@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from be40c4e to f88cc9b Mar 1, 2019
@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from f88cc9b to 481d380 Mar 5, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 7, 2019

/cc @vietj

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 8, 2019

@netty-bot test this please

@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from 481d380 to a3407f5 Mar 14, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 15, 2019

@netty-bot test this please

Copy link
Member

normanmaurer left a comment

LGTM... just one nit

@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from a3407f5 to d770d0b Mar 15, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 15, 2019

@netty-bot test this please

@normanmaurer normanmaurer requested a review from trustin Mar 26, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 26, 2019

@trustin can you have a look as well please ?

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 26, 2019

@netty-bot test this please

@trustin

This comment has been minimized.

Copy link
Member

trustin commented Apr 1, 2019

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

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 1, 2019

@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

This comment has been minimized.

Copy link
Member

trustin commented Apr 1, 2019

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

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 1, 2019

@slandelle can you have a look as well ?

@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Apr 1, 2019

@trustin

enabling by default

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

Copy link
Contributor

slandelle left a comment

2 small nits. LGTM otherwise!

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 of by default to keep backward compatibility

Result:

WebSocket client handshaker to comply with RFC. Fixes #8883.
@kachayev kachayev force-pushed the kachayev:ft-websocket-force-close branch from d770d0b to d6795c4 Apr 2, 2019
@kachayev

This comment has been minimized.

Copy link
Contributor Author

kachayev commented Apr 2, 2019

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

@normanmaurer normanmaurer requested review from slandelle and trustin Apr 2, 2019
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 2, 2019

@trustin PTAL again

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 2, 2019

@netty-bot test this please

@normanmaurer normanmaurer self-assigned this Apr 10, 2019
@normanmaurer normanmaurer merged commit ee351ef into netty:4.1 Apr 10, 2019
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Apr 10, 2019

@kachayev thanks a lot!

normanmaurer added a commit that referenced this pull request Apr 10, 2019
…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.
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.