Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
<Configure id="Server" class="org.eclipse.jetty.server.Server">
<Call name="insertHandler">
<Arg>
<New id="GracefulHandler" class="org.eclipse.jetty.server.handler.GracefulHandler" />
<New id="GracefulHandler" class="org.eclipse.jetty.server.handler.GracefulHandler">
<Set name="shutdownIdleTimeout" property="jetty.graceful.shutdownIdleTimeout"/>
</New>
</Arg>
</Call>
<Set name="stopTimeout"><Property name="jetty.server.stopTimeout" default="5000"/></Set>
Expand Down
3 changes: 3 additions & 0 deletions jetty-core/jetty-server/src/main/config/modules/graceful.mod
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,8 @@ etc/jetty-graceful.xml
# tag::documentation[]
## The timeout, in milliseconds, to apply when stopping the server gracefully.
# 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

# end::documentation[]

Original file line number Diff line number Diff line change
Expand Up @@ -245,10 +245,6 @@ public long getIdleTimeout()
public void setIdleTimeout(long idleTimeout)
{
_idleTimeout = idleTimeout;
if (_idleTimeout == 0)
_shutdownIdleTimeout = 0;
else if (_idleTimeout < _shutdownIdleTimeout)
_shutdownIdleTimeout = Math.min(1000L, _idleTimeout);
}

/**
Expand Down Expand Up @@ -791,4 +787,12 @@ public String toString()
hashCode(),
getDefaultProtocol(), getProtocols().stream().collect(Collectors.joining(", ", "(", ")")));
}

@Override
public void dump(Appendable out, String indent) throws IOException
{
dumpObjects(out, indent,
String.format("idleTimeout=%s", _idleTimeout),
String.format("shutdownIdleTimeout=%s", _shutdownIdleTimeout));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import java.util.function.Function;

import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.server.AbstractConnector;
import org.eclipse.jetty.server.Connector;
import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.HttpStream;
import org.eclipse.jetty.server.Request;
Expand All @@ -39,6 +41,7 @@ public class GracefulHandler extends Handler.Wrapper implements Graceful
private final AtomicLong _requests = new AtomicLong();
private final AtomicLong _streamWrappers = new AtomicLong();
private final Shutdown _shutdown;
private Long _shutdownIdleTimeout;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd use a normal long, and if negative skip processing.
Defaults to -1.


public GracefulHandler()
{
Expand Down Expand Up @@ -74,6 +77,11 @@ public long getCurrentStreamWrapperCount()
return _streamWrappers.longValue();
}

public void setShutdownIdleTimeout(long shutdownIdleTimeout)
{
_shutdownIdleTimeout = shutdownIdleTimeout;
}

/**
* Flag indicating that Graceful shutdown has been initiated.
*
Expand Down Expand Up @@ -131,6 +139,18 @@ protected void doStart() throws Exception
{
// Reset _shutdown in doStart instead of doStop so that the isShutdown() == true state is preserved while stopped.
_shutdown.cancel();

if (_shutdownIdleTimeout != null)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
for (Connector connector : getServer().getConnectors())
{
if (connector instanceof AbstractConnector abstractConnector)
{
abstractConnector.setShutdownIdleTimeout(_shutdownIdleTimeout);
}
}
}

super.doStart();
}

Expand Down