-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
allow configuring web socket close timeout #8189
allow configuring web socket close timeout #8189
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does feel like something that has currently been arbitrarily selected, so making it tunable makes sense.
Reading https://datatracker.ietf.org/doc/html/rfc6455#section-7 the only 2 things that might worry me are
The underlying TCP connection, in most normal cases, SHOULD be closed
first by the server, so that it holds the TIME_WAIT state and not the
client (as this would prevent it from re-opening the connection for 2
maximum segment lifetimes (2MSL), while there is no corresponding
server impact as a TIME_WAIT connection is immediately reopened upon
a new SYN with a higher seq number). 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.
and
To prevent this, clients SHOULD use some form of backoff when trying
to reconnect after abnormal closures as described in this section.
Basically whether setting this to really small values leads to failure to reconnect or DOS from resuming in these abnormal closure situations.
But @swankjesse might know more.
|
Thanks. I figured out the issue with gradle. I had my JVM set to Java 8. It works with Java 21. Now these should be gone. |
Could you explain your use case a bit more? I’d expect a well-behaved server to respond to a close frame immediately, so this 60 second limit shouldn’t be relevant in practice. That said I’m happy with the shape of this PR! |
Small follow-up: #8316 |
Sure. The context is an android application in an (air-gapped) Wi-Fi network environment where the mobile devices are moving and need to inform the users if the connection has been lost. So I hit this timeout in tests where the network connection was flakey and it was not a proper close of the connection; which is where the default comes in. And for my use case setting a much lower timeout (ie 10 seconds) made sense because latency is mostly not really an issue - unless it is and that is exactly what I want to know and inform the users that the notifications might be out of date. |
Oh neat! Thanks for the explanation! |
The default timeout of 60 seconds is too high for one of my use cases. I tried my best to match the existing API. Please feel free to give me feedback on how I can make this as good as possible to increase the likelihood of it being merged.
PS Thanks for the efforts maintaining critical infrastructure like this library.
Edit: Figured it out. My JVM was set to Java 8 SDK. I sadly could not run `./gradlew checks`. I included the errors I got.