Issue #9019 - improve configuration for AbstractConnector shutdownIdleTimeout #14263
Conversation
Signed-off-by: Lachlan Roberts <[email protected]>
…1.x/9019-ShutdownIdleTimeout
Signed-off-by: Lachlan Roberts <[email protected]>
| // Reset _shutdown in doStart instead of doStop so that the isShutdown() == true state is preserved while stopped. | ||
| _shutdown.cancel(); | ||
|
|
||
| if (_shutdownIdleTimeout != null) |
There was a problem hiding this comment.
I did this to preserve existing behavior if the shutdownIdleTimeout is set on the connector directly.
So the GracefulHandler will only override the value if the shutdownIdleTimeout is explicitly set on GracefulHandler.
There was a problem hiding this comment.
If it is explicitly set to -1 (so it's not null), you do not want to enter this if branch.
I'd use -1, see above.
There was a problem hiding this comment.
Well the Javadoc on setShutdownIdleTimeout defines that -1 disables the shutdownIdleTimeout mechansim, which means it does not change the idleTimeout for the graceful shutdown. And a value of 0 means infinite shutdownIdleTimeout.
So if someone wanted to disable the shutdownIdleTimeout, then they would need to explicitly set the value to -1, because the default is 1000ms in AbstractConnector.
| # jetty.server.stopTimeout=5000 | ||
|
|
||
| ## The idleTimeout, in milliseconds, to apply to active connections during a graceful shutdown, or to -1 to disable this mechanism. | ||
| # jetty.graceful.shutdownIdleTimeout=1000 |
There was a problem hiding this comment.
This default does not match what's in the code.
When standalone and not explicitly configured, we want to have the default behavior.
When embedded, we want the same default behavior.
I think a better default would be -1, to maintain backwards compatibility.
There was a problem hiding this comment.
The default behaviour is 1000ms and not -1, for the shutdownIdleTimeout -1 is a special value which means to not change the idleTimeout during the graceful shutdown. The value of 1000ms is the default value from AbstractConnector.
If this property is not set then GracefulHandler.setShutdownIdleTimeout is never called so the value remains null, then we never change the default value from that of AbstractConnector.
| private final AtomicLong _requests = new AtomicLong(); | ||
| private final AtomicLong _streamWrappers = new AtomicLong(); | ||
| private final Shutdown _shutdown; | ||
| private Long _shutdownIdleTimeout; |
There was a problem hiding this comment.
I'd use a normal long, and if negative skip processing.
Defaults to -1.
| // Reset _shutdown in doStart instead of doStop so that the isShutdown() == true state is preserved while stopped. | ||
| _shutdown.cancel(); | ||
|
|
||
| if (_shutdownIdleTimeout != null) |
There was a problem hiding this comment.
If it is explicitly set to -1 (so it's not null), you do not want to enter this if branch.
I'd use -1, see above.
Closes #9019
Currently setting the idleTimeout on the connector to 0 will also set an infinite shutdownIdleTimeout, which can be unexpected.
setIdleTimeoutmethod from modifying the shutdownIdleTimeout, so it is no longer order dependent in which order you callsetShutdownIdleTimout()andsetIdleTimeout().