Review Jetty-12 DelayedHandler

[gregw]

Jetty version(s)
12

Enhancement Description
Jetty-12 has generalized the previous jetty delayed dispatch mechanism. Currently there are Handlers that can delay dispatch until:

• some content is available
• form content has been read and converted to a form
• multipart content has been asynchronously read and converted to parts.

However, it is possible that a deployment might need multiples of these behaviors and it is undesirable to have 3 such handlers stacked up. Thus we should consider refactoring so that there is a single DelayedHandler that can optionally perform various types of delay depending on content-type.

Furthermore, whilst the downstream handling of forms appears to have been updated to use any form read by this handler, the same has not been done for multiparts. The same pattern of looking for the processed content in the request attributes should be applied to parts.

[gregw]

I have a draft of this, but it is waiting for #9035 to be merged

[gregw]

#9056 has improved the implementation and merged the multiple handlers into a single one.
However, the API for extending it is not satisfactory and more thought is needed, so this issue should remain open until a better API is developed.

[gregw]

@sbordet Currently the problematic API is:

    protected DelayedProcess newDelayedProcess(boolean contentExpected, String contentType, MimeTypes.Type mimeType, Handler handler, Request request, Response response, Callback callback)

So first question: Should this handle be able to delay for non-content related purposes? We have no real use-cases, but we do have a test case for that, hence the need to pass contentExpected.

A good first simplification could be to rename the handler to DelayedContentHandler and to remove this parameter, so that if there is no content, then there is never any delay:

    protected DelayedProcess newDelayedProcess(String contentType, MimeTypes.Type mimeType, Handler handler, Request request, Response response, Callback callback)

We are passing both contentType and mimeType so we can switch efficiently on the later to find cases for known types, but we also need the former so we can extract a boundary from it for multipart.
It is a 1 liner to get the mimeType from the contentType, so perhaps we just move that into the method ?:

    protected DelayedProcess newDelayedProcess(String contentType, Handler handler, Request request, Response response, Callback callback)

Alternately, we could have a DelayedProcessFactory pattern:

    protected DelayedProcessFactory getDelayedProcessFactory(String contentType);
    interface DelayedProcessFactory
    {
        DelayedProcess newDelayedProcess(Handler handler, Request request, Response response, Callback callback);
    }

But multipart and other delays would need to wrap the factory to remember the boundary. I'm not sure the extensibility needs of this class justify a factory.

On balance, I think the version that just takes the String content-type is a good simplification, so long as we agree that this handler only delays content and no other reason?

[github-actions]

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

[gregw]

@sbordet I'm bumping this to 12.1.0 and think we should consider re-introducing the built in HTTP/1 delayed dispatch (on by default) and making this handler less general purpose.

[gregw]

@sbordet @lorban @lachlan-roberts I'm going to reintroduce delayed dispatch to HttpConnection. Can you guys look at doing the same for h2 and h3. I think the DelayedHandler should then only be used for asynchronously reading entire contents before dispatch (probably renamed)