Jetty.project: Server.join not working when used with ExecutorThreadPool

[avshenuk]

Jetty version
9.4.28.v20200408
Java version
OpenJDK Runtime Environment (build 11.0.6+10-LTS) by Azul Systems, Inc.
OS type/version
MacOS Catalina 10.15.3
Description
Hi guys,

Server.join is not working properly when given ThreadPoolExecutor. Basically, the implementation of ExecutorThreadPool calls awaitTermination with stopTimeout as opposed to Integer.MAX_VALUE in the previous versions and that's not really what should be done here. Documentation of awaitTermination:

Blocks until all tasks have completed execution after a shutdown request, 
or the timeout occurs, or the current thread is interrupted, whichever happens first.

Basically, it doesn't care if the shutdown has been requested and will always wait at most stopTimeout.

Very simple reproduction example:
[prints "start" and "end" and exits immediately]

        ExecutorService executorService = Executors.newCachedThreadPool();
        Server server = new Server(new ExecutorThreadPool((ThreadPoolExecutor) executorService));
        server.setStopTimeout(0);
        server.start();
        System.out.println("start");
        server.join();
        System.out.println("end");
        System.exit(0);

vs
[prints "start" and hangs forever]

        Server server = new Server();
        server.setStopTimeout(0);
        server.start();
        System.out.println("start");
        server.join();
        System.out.println("end");
        System.exit(0);

I think the simplest elegant fix from the Jetty side (we can also set stopTimeout to Integer.MAX_VALUE as a hack) would be to add logic into ExecutorThreadPool.join from QueuedThreadPool:

public void join() throws InterruptedException
    {
        synchronized (_joinLock)
        {
            while (isRunning())
            {
                _joinLock.wait();
            }
        }

        _executor.awaitTermination(getStopTimeout(), TimeUnit.MILLISECONDS);
    }

plus

synchronized (_joinLock)
        {
            _joinLock.notifyAll();
        }

in doStop.

And thank you so much for such a great and very valuable project!

[lachlan-roberts]

Looks like this issue has been fixed in jetty-10.0.x with PR #4482 as part of a larger work on graceful shutdown, but these changes have not been brought back to 9.4.x.

The fix was to call change ExecutorThreadPool.join() to use Long.MAX_VALUE instead of using getStopTimeout().

@Override
public void join() throws InterruptedException
{
    _executor.awaitTermination(Long.MAX_VALUE, TimeUnit.MILLISECONDS);
}

The current behaviour looks like an incorrect use of the stopTimeout, so I think we should make this same change in 9.4.x as well.

[lachlan-roberts]

Fixed in PR #4882