Idle timeout is ignored if callback is not completed

[joakime]

Jetty version(s)
Jetty 12

Description
If the Handler.process() Callback is not completed, the Idle timeout mechanism is ignored / doesn't happen.

Is this as intended? or a relic of the Servlet API behavior that slipped into core?

[joakime]

Discovered during testing of Graceful shutdown.

We force the EndPoint.setIdleTimeout() to be lower.
Can we also have the idle timeout to not be ignored?

[joakime]

@gregw what do you think?

[joakime]

So, if a Handler.process() has a bug and doesn't complete the Callback, that means the connection / endpoint is not ever cleaned out via an Idle Timeout.

[joakime]

I wonder if we can have a 2 stage Idle Timeout?

1. First stage, the way it works now.
2. Second stage, if the idle timeout has occurred X times, we force IdleTimeout and close.

[joakime]

Maybe we should add some information to the dump to know time taken for ...

• when the process() returned
• when the callback completed
• the state of the HttpChannelState?

[joakime]

Consider a possible use of the existing Request.addErrorListener(Predicate<Throwable>) as a good place to handle this.

[joakime]

There are 4 use cases to consider here.

1. Idle Timeout during Read
2. Idle Timeout during Write
3. Idle Timeout while in handling (no read/write active?), but callback not yet completed, how to notify application?
4. Idle Timeout while in handling (no read/write active?), callback not yet completed, jetty handles a proper closure of the connection, and completing the callback.

[gregw]

This has been substantially improved by #9905, but more work is needed to be done after beta2

[sbordet]

@gregw I am moving this issue to 12.0.2.

My interpretation of the situation of your last comment is that if a failure happened asynchronously, and the application is not notified of the failure right away, then the application is in a state where it must still do something.

At minimum should complete the callback, but perhaps doing a read (which will see the failure), or doing a write (which will see the failure).
If the application just succeeds the callback we can just try to write a minimal response, and if we fail doing so, well we tried.

To say that I don't think an asynchronous failure should complete the callback.

In this light I'm keeping the issue open for further discussion.

[gregw]

@sbordet after our discussion on this yesterday, we both agreed that the following handler kind of breaks the handler contract by not arranging for the callback to be completed:

        new Handler.Abstract.NonBlocking()
        {
            @Override
            public boolean handle(Request request, Response response, Callback callback)
            {
                return true;
            }
        };

However, in the current 12.0.[01] releases, this handler does not hang forever because eventually an idle timeout comes along, is treated as a failure (no IO and no idle listener) and the failure handling currently will fail the callback if there is no IO and no failure listener.

We agreed that something like the following is more "correct":

        new Handler.Abstract.NonBlocking()
        {
            @Override
            public boolean handle(Request request, Response response, Callback callback)
            {
                request.addFailureListener(callback::failed);
                return true;
            }
        };

As this explicitly adds a failure listener that will fail the callback. If the application wants to do more complex handling of failures, then they can just add a more complex listener.

But for the most part, handlers are not going to want to do complex failure handling and for many wrapping handlers it is pretty tough to do so anyway. So do we really want to have a setup where handlers that do not want complex error handling have to put in a request.addFailureListener(callback::failed); on every branch that they decide to handle the request themselves... but not on any branch where they have deferred the handling to a nested handler?

Perhaps we just amend "the contract" to say that without an explicitly added handler for failures, there is an implicit request.addFailureListener(callback::failed); ?

[sbordet]

@gregw I don't think we can do what without stepping on the application's toes.

For example, the application wants to write a specific response in case of errors; I'm thinking of those applications that read/write JSON and even in case of errors write a 200 OK with an "error" JSON.

Their client will expect JSON, and us failing the callback under the covers with a generic 500 with a body that is not JSON is something we need to have developers opt-in rather than hard-code.

How about a FailureHandler that calls request.addFailureListener(callback::failed) that can be set as outermost?

[lorban]

@sbordet wouldn't your proposal still leave a gap open, i.e.: the handler's callback may not always be completed?

Trying to fit the shoes of an application writer whose app only speaks JSON, if I'd forget to call request.addFailureListener() and the worst happens I'd very much rather have an HTML error reported back rather than have a request forever in limbo.

[gregw]

@gregw I don't think we can do what without stepping on the application's toes.

How so? Any application that cares about error handling will add failure listeners and then we do nothing for them.

The proposal for an implicit failure handler only affects apps that don't set a failure listener. The question is, when an app doesn't set a listener, is it because they really want to continue if there is a failure, or they just didn't think about it. I'm guessing more of the later.

[sbordet]

@gregw @lorban I see this case similar to AsyncContext.setTimeout() which the Servlet spec defaults at 30 seconds and just gives every developer headaches because they don't know about the feature, and some thread timing out step of their toes.
We had so many issues reported in the past because of this, and usually the solution is "just disable the AsyncContext timeout".

That we hardcode the failure into our implementation is a point of no return, as we would not be able to change the behavior in the future.
We would force people that do not want that behavior to act (adding a failure listener) to counter the hardcoded behavior, like you have to do with AsyncContext.
And once they do that, we won't be able to change the handcoded behavior of failing the callback in the future.

I like the fact that we can compose Handlers to offer this additional functionality, like we tried to do with DelayedHandler (rather than having the feature hardcoded like we had in 11).
Furthermore, people would be able to write timeout-based Handlers that would fail the callback after a timeout, no matter what child Handler is added to the tree (so, organization Handlers vs some department Handlers).

Seems more flexible to just add features, rather than knowing that there are some that you have to neutralize, and then adding yours.

[gregw]

I don't think that composing handlers work. Many handlers will have complex logic about how they will handler a request, with multiple possible handler methods being possible, including delegating to other handlers. For example, consider a security handler that might: redirect to a login page; send a failure to login; send a challenge for credentials; let the request pass through with deferred auth; let the authenticated request pass through. Each of those will have different error handling and some of it will be unknown. Just having a composed handler that adds in an error handler like request.addFailureListener(callback::failed) is not a solution since if the request is passed to a handler that explicitly handles errors, then it will not know (or be able) to remove that failure handler.

The difference to AsyncContext.setTimeout() is that setting a timeout replaces any default, whilst adding an error listener does not remove any previously added listeners!

The beauty of the current solution is that the callback is failed for the 99% of handlers that will not explicitly handle errors, but for the 1% that do handle errors, then default behaviour will be automatically removed.

Only the handler that has taken responsibility to generate the response and complete the callback should add an error listener that will fail the callback. Other handlers might have need of an error listener, but if they were not generating the response, then that listener should not fail the callback.

[joakime]

Can we close this? PR #10418 has been merged.

[sbordet]

Fixed by #10844 and further refined by #10920.