Fix #356 - expand idle timeouts #377
Conversation
|
I've tried to clarify the language. This change was prompted by a user request. The scenario was something like the server was sending messages every 10s and the client was sending messages every minute. They wanted to be able to detect that the client had stopped sending messages and the combined read/write idle timeout couldn't do that. Hence the request for separate read/write idle timeouts. I did think about ways to remove the combined read/write idle timeout and/or convert it to the individual read and write timeouts but logically it just doesn't work. |
|
This looks ok by me. I like the clarification for zero and negative values. The cons for this PR is that an extra thread would get consumed, currently a thread checks for read and write timeout, now there would be one thread for each (if read and write timeout differs). This can be an issue in an application server that defines a pool of threads and the default is very low, The customers are often used to having pools with just one or two threads, unfortunately. |
|
Eclipse Jetty is 100% async, any blocking concepts (like what exists in the Servlet spec) are faked. |
|
A connection has an overall idle timeout, and activity resets the overall timeout. Hell, even Java itself doesn't have this kind of split on read vs write idle timeout. |
|
I also don't like what this PR implies for the behaviors on WebSocket over HTTP/2 (Which Eclipse Jetty supports) and WebSocket over HTTP/3 (Which we have prelim support for). |
| * | ||
| * @param milliseconds the number of milliseconds. | ||
| */ | ||
| void setMaxIdleTimeout(long milliseconds); | ||
|
|
||
| /** | ||
| * Return the number of milliseconds before this conversation may be closed by the container if no messages are | ||
| * received in that time. A value that is zero or negative indicates that this timeout will not be used. The general |
There was a problem hiding this comment.
Complete message? (all data frames?)
Partial message? (some data frames? an incomplete data frame?)
Control frame?
Reset on receive of traffic?
Or reset on delivery of data frame? or any frame? or even auto-fragmented frame?
What if the connection has higher level activity not within websocket (eg: http/2 stream specific details, or http/3 stream specific details)?
|
|
||
| /** | ||
| * Return the number of milliseconds before this conversation may be closed by the container if no messages are | ||
| * sent in that time. A value that is zero or negative indicates that this timeout will not be used. The general |
There was a problem hiding this comment.
Sent by what?
The application has requested a send?
The extension has enough information to send something?
The websocket connection has sent it?
The HTTP/2 stream has sent it?
The TCP connection has sent it?
The HTTP/3 stream has sent it? Do we wait for confirmation on HTTP/3 of the send?
|
There's a reason why there's no separation of read vs write idle timeout on low level Socket connections. |
|
I agree clarifying the language around the current idle timeout would be helpful. Since this is a WebSocket level timeout, I think it should be defined in terms of WebSocket and not any underlying protocol. How about something like:
as a starting point? While this clarification is useful, it does not address the original feature request from the user. If separate read/write idle timeouts are not acceptable how do you propose we meet the original requirement set out in #356? |
|
Lets continue to talk in #356 |
|
Closing this PR. See #383 for the replacement. |
No description provided.