Jetty-12 HttpContent should have an async API

[gregw]

@lachlan-roberts has recently been working on the content caching mechanism for Jetty-12. He's been receiving contradictory advice: I've been advocating that HttpContent should have an async API (maybe even just implement Request.Processor, but I think others have been advocating for keeping the blocking getBuffer() approach.

So I'm opening this issue to capture some of the arguments/discussions, but ultimately I think we need to have a meeting to pick a single direction and go with it.

The ResourceService in Jetty-12 has been greatly simplified, primarily by moving a lot of functionality out of special case handing into ContentFactory implementations and wrappers. Thus caching, cache invalidation etc. are all applied by wrapping / configuring / replacing the ContentFactory. I think this is a great approach, but without an async API it means that only blocking behaviours can be moved into a ContentFactory. Currently, we still have behaviours for directory listing, welcome files, precompressed content, range request etc. handled as conditional behaviours in the ResourceService. If HttpContent was a Processor most of these complexities could be moved out to ContentFactory wrappers and only applied if actually needed/used.

A good example was that @sbordet has a use-case for handling welcome files with a redispatch through the context handlers: the case where a welcome file might be index.php and to serve it it needs to be redispatched via the handler that will proxy a php to a different process. With the current code, the ContentFactory will produce a HttpContent that represents the source code of the index.php file, probably even blocking for a little while in the constructor to load a ByteBufffer with the content that will never be used and creating PreEncodedHttpFields that are not relevant! To handle this case, we must add a special welcome file mode to the WelcomeFactory, WelcomeAction and to the ResourceService itself. This adds complexity even if a deployment doesn't need this feature.

If instead, the HttpContent was a Request.Processor, then a WelcomeFileContentFactory wrapper could replace any directory HttpContents it receives from it's nested factory with a processor that would generate the contents of the directory. This might be a redirect, generate a xhtml listing, or it may be a fully async redispatch through the handler tree, ultimately talking to a PHP server in another process. Better yet, this could be done before the caching content factory, so the welcome file content could be cached.

Better yet, new ways of handling directories and/or welcome files can be added simply by adding new ContentFactory wrappers rather than adding more ifs and buts into the already complex ResourceService.

As far as I can tell, the counter argument is that this is "optimising the slow path". I don't think that is the case because the async API is perfectly capable of being used to implement the simple direct behaviour that we currently have which is (simplified):

    public void doGet(Request request, Response response, Callback callback, HttpContent content) throws Exception
    {
            if (content.getResource().isDirectory())
            {
                sendWelcome(content, pathInContext, endsWithSlash, request, response, callback);
                return;
            }

            writeHttpContent(request, response, callback, content, reqRanges);
    }

    protected void sendWelcome(HttpContent content, String pathInContext, boolean endsWithSlash, Request request, Response response, Callback callback) throws Exception
    {
        // process optional Welcome behaviors
        if (welcome(request, response, callback))
            return;
        sendDirectory(request, response, content, callback, pathInContext);
    }

    protected void writeHttpContent(Request request, Response response, Callback callback, HttpContent content)
    {
        try
        {
            ByteBuffer buffer = content.getBuffer();
            if (buffer != null)
                response.write(true, buffer, callback);
            else
                new ContentWriterIteratingCallback(content, response, callback).iterate();
        }
        catch (Throwable x)
        {
            callback.failed(x);
        }
    }

I think this should just become:

    public void doGet(Request request, Response response, Callback callback, HttpContent content) throws Exception
    {
        httpContent.process(request, response, callback);
    }

and by the miracle of object oriented programming, the content would serve itself with the best technique available. If no welcome files are configured, then there is no welcome file handling code in the hot path. If welcome files are configured the content provided for a directory resource will either already be the welcome file, or be a processor that can run the welcome file logic.

If we wanted to have a simple cache that blocks loading the content into a buffer then we could had the implementation would end up something like:

    public class CachedHttpContent extends HttpContent.Wrapper
    {
        private final ByteBuffer _content;

        public CachedHttpContent(HttpContent content)
        {
            _content = load(content);
        }
    
        public void process(Request request, Response response, Callback callback)
        {
            response.write(true, buffer, callback);
        }
    }

If that is the style of cache we want to have, there is no significant cost to this API approach, as it simply replaces if statements with virtual method call. There may be some extra complexity in loading the wrapped HttpContent into a ByteBuffer, as we will need a fake request,response,callback, but that can be done in a utility class and will be generally useful anyway. This is not "optimizing" the slow path. This is generalising the hot path.

But by organising the API in this way, we can:

• further simplify the minimal resource service, which now doesn't care about caching and we can do the same with welcome files etc.
• decide to lazily load cached buffer, either keeping a list of calls to processes to complete, or forwarding them to the wrapped processor, until the content is loaded.
• do arbitrary redispatching of content without any extra complications in the ResourceService
• Cache directory and welcome file contents. Perhaps even using a cache (third party?) that actually looks at headers such a Vary and Cache-Control
• allow for arbitrary more complex behaviours in future.

In fact, we could almost go even further, and have the HttpContent as a Request.Processor be returned from the handle method of the ResourceHandler!?!?

In short, in Jetty-12 it is the Request.Processor API that we use to serve content. We wrap the processor to add behaviours like compression. We should use this same approach for static as well as dynamic content.

[gregw]

So to take my proposal to it's logical extreme, we should change the method in ResourceHandler from:

    @Override
    public Request.Processor handle(Request request) throws Exception
    {
        HttpContent content = _resourceService.getContent(Request.getPathInContext(request), request);
        if (content == null)
            return super.handle(request); // no content - try other handlers

        return (rq, rs, cb) -> _resourceService.doGet(rq, rs, cb, content);
    }

to

    @Override
    public Request.Processor handle(Request request) throws Exception
    {
        HttpContent content = _resourceService.getContent(Request.getPathInContext(request), request);
        if (content == null)
            return super.handle(request); // no content - try other handlers

        return content;
    }

We may even save on the allocation of a lambda this way :)

[lachlan-roberts]

@gregw HttpContent is in jetty-http module so does not have access to Request and Response classes, so it cannot have the httpContent.process(request, response, callback) method you propose, and it cannot implement Request.Processor.

Do you think that HttpContent should be moved to jetty-server?

[gregw]

@gregw HttpContent is in jetty-http module so does not have access to Request and Response classes, so it cannot have the httpContent.process(request, response, callback) method you propose, and it cannot implement Request.Processor.

Do you think that HttpContent should be moved to jetty-server?

I'm just doing big picture here.... details like that are for you :)

[gregw]

So thinking out loud here about how this could work if HttpContent implements Request.Processor (or has a signature that can be returned as a method pointer as a Processor).

At the simplest level, we would have a ResourceHttpContentFactory that would return a ResourceHttpContent which would implement process to set the response headers and then use an Iterating callback to read & write the content a buffer at a time. The buffer pool and buffer size would come from the RHCF. Content smaller than a single buffer would be read into that buffer and then written in a single last write. For directories resources, the process could produce the xhtml of the directory listing; or send a 403 if the RHCF was configured to do so. Welcome files can be implemented by later wrapping of the HttpContent.

We could then have a FileMappingHttpContentFactory that optionally wraps RHCF (or any other CF). It would look at the HttpContent obtained from it's wrapped factory and if HttpContent.getResource().getPath() returns something that is mappable and if the content meets other criteria (i.e. size, type?) then the HttpContent would be wrapped as a FileMappedHttpContent which would implement process by setting the response headers (obtained from the wrapped HttpContent) and then writing the mapped file buffer. I think it is likely that lots of contents will need to set the response headers, so a static utility method that uses the getter on the HttpContent API is likely to be needed. Any other content is passed through unaltered by the FMHCF.

A WelcomeFileHttpContentFactory would wrap another CF and it would intercept any directory HttpContents it sees. For directory contents, it would iterate through the welcome files list (configured in factory which might take it from the resource service/handler), create new resources and ask it's wrapped CF for a content. If it gets back a content, then it can return that content as the processor; return a processor that redirects to it; or return a processor that re-handles the request.

Any HttpContent can be wrapped as with a ConditionalHttpContent, so that the process method would check the If-Xxxx- headers of the request and if they are met, send the appropriate 3xx response. Otherwise the process would be passed on to the wrapped content to be generated normally.

With regards to caching, there are two types of cache we could use/implement. Firstly, because the HttpContent is-a Request.Processor, then all the response headers are available to it, so we could implement a full HTTP cache that is controlled by Cache-Control headers. This would then then be able to cache both static and dynamic content. It could be put above any compression CFs as it would be able to look at the Vary headers etc. However, such a cache should probably be done as a pure handler rather than as part of the resource service. (it should probably also use a third party library as such caches are very complex).

Secondly, we could have a ResourceCacheHttpContentFactory, that would only be interested in caching static content... or more precisely content that has a non null getResource().getPath(). The processor returned for such contents would not necessarily be just the cached content, but rather a processor that is able to lookup a cache for the content and if not found then call the wrapped processor to serve it normally. Such a processor can then either hold up other request being processed waiting for a cache to be filled, or pass them through until such time as the cache entry is available, when it then will be used. The cache would be able to put some contents (e.g. FileMappedHttpContent) directly into the cache, whilst it may have to create others (e.g NotFoundContent or a BufferedHttpContent that would be created by sending a fake request/response to the nested processor).

A PrecompressedHttpContentFactory would work kind of like the welcome file one, but its process would need to look at request headers to see what contents to look for in its nested factory. Such a factory could not be below a cache that did not pay attention to the Vary header, else one type of precompressed content could end up getting cached.

[gregw]

@lachlan-roberts @sbordet @lorban ^

[sbordet]

The problem is that HttpContent is in jetty-http so any possible signature would be to only depend on jetty-http something like:

HttpContent.write(HttpFields requestHeaders, HttpFields responseHeaders, Sink response, boolean last, ByteBuffer content)

which is quite long and cumbersome, and it needs to be re-wrapped as a Processor anyway.
And it's going to be used by the server only, so it feels wrong to be there (TBH even HttpContent feels wrong there, because it's never used by the client -- so perhaps we should start moving HttpContent).

To write directories, we would need to pass to the HttpContent some configuration about whether this is allowed (which already resides elsewhere), and kind of pollute jetty-http with HTML, JSON, etc.

Also, I don't like that you now rely on getPath() to make decisions about how to process the content.

For example, the CachingHCF would be hit, cache miss, delegate to nested HCF, which returns a HC.
Now, because there is no getBuffer() anymore (which was the main thing that CachingHCF would care about) now the CachingHCF only has getPath() so now CachingHCF bypasses whether mechanism was implemented by the returned HC to actually write the ByteBuffer -- for example it would not file-map it.

Any non-Path based Resource would now be excluded from all the wrapping HCF (for example, MemoryResource).

I am skeptical about this approach, even with HttpContent moved to jetty-server.

[gregw]

The problem is that HttpContent is in jetty-http so any possible signature would be to only depend on jetty-http ...

This is not an issue. This design is only relevant to the server anyway, so it just needs to be in ResourceService, which could be written in terms of something like:

    interface HttpContentProcessor extends Request.Processor
    {
        HttpContent getHttpContent();
    }

Or it could be a HttpContent extension that provides a Processor or that is-a Processor. Eitherway, HttpContent could be left as is (well perhaps remove the mostly broken getBuffer() or at least remove all policy from it).

Ultimately the project structure is there to support good design, not to limit design choices.

TBH even HttpContent feels wrong there, because it's never used by the client -- so perhaps we should start moving HttpContent.

Oh if HttpContent is not used by the client, then definitely let's consider moving it. As I said, this is a second order impl issue and should not effect the design.

For now, I'll just call it HttpContentProcessor to make it clear... ultimately this might just be HttpContent

To write directories, we would need to pass to the HttpContent some configuration about whether this is allowed (which already resides elsewhere), and kind of pollute jetty-http with HTML, JSON, etc.

Not necessarily. The policy decision could be made by the factory when the HttpContentProcessor is created/returned. If directories are not allowed then the factory can either return null (for a 404 or to allows something else to have a go) or it can return a ForbiddenProcessor that just produces a 403 response (this processor can be cached to make the decision faster in future).

You only need to give the HttpContentProcessor access to configuration if it needs to make a policy decision based on an individual request (e.g. when selecting a pre compressed content). But in such cases, it can easily access the factory for configuration (or be injected by the factory). The factory itself can either be directly configured or look to the ResourceService instance for configuration (or be injected by the ResourceService).

Again, configuration is important, but a second order issue to good design of how to serve content efficiently.

Also, I don't like that you now rely on getPath() to make decisions about how to process the content.

It is not relied on. Implementations are perfectly able to make decisions entirely on the basis of the intercepted/wrapped request/response/HttpContent. I gave the example of a cache that can cache any content and which looks at Cache-Control and Last-Modified headers to manage itself. However, whilst such caches are good and powerful, there are optimisations available if we know the content is coming from the file system. So this design has the option to look for a path, and if available then creating a path specific processor.

This is no different from the current code, which for example, needs to know there is a path before deciding to try to memory map the file.

At least this design allows for path independent implementations because it has access to the response headers of the wrapped content. The getBuffer() implementations can not support that.

For example, the CachingHCF would be hit, cache miss, delegate to nested HCF, which returns a HC. Now, because there is no getBuffer() anymore (which was the main thing that CachingHCF would care about) now the CachingHCF only has getPath() so now CachingHCF bypasses whether mechanism was implemented by the returned HC to actually write the ByteBuffer -- for example it would not file-map it.

It doesn't only have getPath(). It has process, so it can get the entire content and all it's meta data; it has the other HttpContent methods so it can get selected meta data; it has the Resource API for even more abstracted meta data (isDirectory(), isReadable() etc.). It only needs to use path API if it wishes to do path based things or perhaps if its policy is to only cache things with paths.

Any non-Path based Resource would now be excluded from all the wrapping HCF (for example, MemoryResource).

No - it would only be excluded from the wrapping HCF's that have path specific behaviours.

I am skeptical about this approach, even with HttpContent moved to jetty-server.

Yet getBuffer remains fundamentally broken. It is required to make policy decisions about what kind of buffer to return based on no information about the caller or why it is being called. If the buffer it needs to return is not immediately available (e.g. because a re-dispatch is needed) then it cannot handle that asynchronously and thus pushes complexity elsewhere.

[sbordet]

Yet getBuffer remains fundamentally broken. It is required to make policy decisions about what kind of buffer to return based on no information about the caller or why it is being called.

I'm still not convinced about this.
Remind me what's broken, because right now we have a fully performant, all cases supported, implementation.
The only problem I remember with the current implementation is that it can be misconfigured it in one specific case, which I feel too weak of a reason to redesign it.

It has process, so it can get the entire content

So, ResourceService calls some process() on a CachingHC, which wraps a ResourceHC.
I understand that ResourceHC.process() actually writes the resource to the response, am I right?
If that's true, how does the CachingHC get that buffer to cache it?

And process may not be able to get the entire content, as it may be "chunked".

What am I missing?

[gregw]

I'm still not convinced about this.

Yeah well, the convincing needs to go both ways!

I'm not convinced about the currently checked in code and I only acquiesced to several PRs being merged for expediency as lack of static resources was holding up other development. We cannot allow incumbency in an alpha branch to be the determining factor of our final design.... else a lot more PRs are going to be -1 reviewed until they are perfect and convince everybody. That is not a productive work cycle.

So I don't need to make the case to replace the current alpha implementation. The new implementation needs to make the case for why it belongs in the final code base - other than expediency.
I'm not convinced by the alpha impl and if my alternative suggestions are not convincing to you, then we need to keep searching for a solution that is convincing for all.

What am I missing?

You're missing that we should have an API that is extensible, flexible and efficient. It is not good enough to have an API that is just good enough for basic cases.

The problems with getBuffer() API include:

• it is not fully extensible as it's design forces anything complex to be done by adding behavior into ResourceService and/or ResourceHandler. Welcome files, directory listings, precompression all need explicit handling in the service and/or handler. Just to deploy our own simple website, we needed to add special handling and extra modes into ResourceService. I was "sold" on this big refactor on the basis that complexity was being taken out of the service/handler and being put into extensible content factories, with a deployment only needing to deploy the complexity that it uses. This is a good design, but getBuffer means that it can only be done for some types of resource and only for some style of serving. If we want it fully extensible, then we need to be able to do complex things in content factories and the contents that they create, so a better API than getBuffer.
• there is confusion about exactly what getBuffer() is meant to be doing, hence the disagreements about if it should always return non-null, sometimes return null based on policy or only return null if physically impossible to load a buffer in. The result is that policy decisions are smeared between factory, HttpContent constructors and getBuffer implementations. This confusion cannot be solved, because there is no single reason for calling getBuffer and it's behavior needs to be different depending on the caller. It just so happens that for the 95% case it is OK. I don't think that is good enough.
• it is blocking, and the whole point of jetty-12 was to make a fully async server.
• it's design forces a lot of behavior to be done in the HttpContent constructor, which increases the time that a getContent call takes and thus either increases lock contention and/or increases stampede problems.
• It does not allow caching for any content that does not have a singular non varying buffer e.g directory listings and/or welcome files.

I understand that ResourceHC.process() actually writes the resource to the response, am I right?

No. It writes the content to "a" response, which is not necessarily "the" response. The whole Content.Sink approach we have developed is precisely to allow content to be easily intercepted asynchronously. Ditto with response wrappers for intercepting meta-data.

If that's true, how does the CachingHC get that buffer to cache it?

As described above, there are multiple options for a cache. One can be file system aware, so it can use getResource().getPath() APIs to buffer files directly from the file system. Alternately we could have a full HTTP cache of arbitrary content, then we can intercept the response Content.Sink and put that into a buffer - i.e. make a fake request/response to pass to the processor to obtain the content and all of its meta-data in the response headers. I think the first type is what we should initially provide, but the design allows for the full cache to be implemented (but probably best to get a 3rd party library rather than re-implement a full HTTP cache).

So is this proposal not workable? I think it embraces all the good work we have done to make Request.Processor a good flexible, extensible async API. It also embraces the redesign goal for the resource service of moving complexity out of the service/handler and into extensible content factories.

What am I missing?

[lorban]

@gregw If my understanding is correct, you're proposing to manage static content with a mechanism vaguely similar to the the 11.0.x handler one: pass the request/response/callback trio to some ContentFactory and let the latter serve the request if it can.

This raises two questions in my mind:

• Is that going to be easier to write/maintain than the existing list of if/else we currently have in ResourceService?
• Is a delegating model the right one, or should we use a list of ContentFactory instead?

I'm inclined to believe the answer to the 1st one is yes, meaning we should move on with this idea. But I really am on the fence about the 2nd one.

[gregw]

@gregw If my understanding is correct, you're proposing to manage static content with a mechanism vaguely similar to the the 11.0.x handler one: pass the request/response/callback trio to some ContentFactory and let the latter serve the request if it can.

@lorban not really.... at least i don't think of it in that way.

I'm proposing to embrace the new design. A HttpContent is obtained from content factory more or less as done now in the alpha. But then letting the content serve itself rather than the service serving the content (by making a policy choice about calling getBuffer or creating an iterating callback that uses the channel from the resource).

This would be done with a process like API. Because it has the full request and response, it is more powerful than getBuffer and will allow such things as welcome files and pre-compression to be done in content wrappers rather than the service.

The HttpContent (or it's wrappers) would make them policy decisions with full access to request, response, factory, service, resource and any cached data or meta data.

This raises two questions in my mind:

• Is that going to be easier to write/maintain than the existing list of if/else we currently have in ResourceService?

Well the previous resource service was a great big blob of if buts and elses. It was hard to understand and near impossible to maintain. It certainly was not easily extensible. It was your idea to simplify by moving the caching out to content factories and content wrappers - it was a good idea as not only does it let us deconstruct our own caching, it opens the possibility for more complex caching (3rd party?) to be plugged in.

I'm just proposing we follow that example/pattern for other special case resource handling (welcome files, pre compression, re-dispatch, things we have not thought of yet).

• Is a delegating model the right one, or should we use a list of ContentFactory instead?

Pass. That detail can be sorted out by whoever actually does there implementation.

I'm inclined to believe the answer to the 1st one is yes, meaning we should move on with this idea. But I really am on the fence about the 2nd one.

I agree that decomposing into factories and content wrappers will be easier to maintain. I'm flexible on exactly what kind of reassembly model is used.

[sbordet]

@gregw it is currently not true that HC has access to request and response because it lives in jetty-http and not in jetty-server. At best it has access to source and sink and HttpFields.

Let's say that we move it to jetty-server.

A cache implementation will have to wrap the response with some complex logic about either aggregating buffers into a single one that it is going to be cached, or store them aside and always use multiple writes to write them out.

Also, we would need to write Servlet specific HCFs for every possible HCF that needs to perform redirects/writes because we want to use Servlet APIs to do those, not core APIs.
Currently, we have only one such extension point, the ResourceService.

Pseudo-code for file mapping:

class FileMappingHC extends HC.Wrapper {
  void process(Request req, Response res, Callback cbk) {
    if (couldMap == null) {
      buffer = tryMapping(getWrapped());
      couldMap = buffer != null; 
    }
    if (couldMap == Boolean.TRUE) {
      // TODO: must have a Servlet version for this.
      res.write(true, buffer.slice(), cbk);
    } else {
      getWrapped().process(req, res, cbk);
    }
  }
}

Pseudo-code for caching:

class CachingHC extends HC.Wrapper {
  void process(Request req, Response res, Callback cbk) {
    if (couldCache == null) {
      res2 = wrapResponse(res);
      cbk2 = Callback.NOOP;
      getWrapped().process(req, res2, cbk2);
      // Now we have a buffer stored in res2.
      couldCache = canBeCached(res2.buffer);
      if (couldCache) {
        // TODO: also cache headers that could have been "written".
        buffer = res2.buffer;
      }
    }
    if (couldCache == Boolean.TRUE) {
      // TODO: must have a Servlet version for this.
      res.write(true, buffer.slice(), cbk);
    } else {
      // TODO: replace this with NonCacheable(this) in the cache?
      getWrapped().process(req, res, cbk);
    }
  }
}

[gregw]

@gregw it is currently not true that HC has access to request and response because it lives in jetty-http and not in jetty-server. At best it has access to source and sink and HttpFields.

I didn't say it did? I'm OK with it being moved, or if the ResourceService is written in terms of a HttpContentProcessor API that has the content, the resource, the request, the response and the callback. Implementation detail.

A cache implementation will have to wrap the response with some complex logic about either aggregating buffers into a single one that it is going to be cached, or store them aside and always use multiple writes to write them out.

Why? I'm sure we already have code that can act as a Content.Sink' and aggregate all the writes into a buffer (or sequence of buffers - implementation detail). If we don't, then it is a fairly simple utility class to write. I don't see it as very complex and it hardly qualifies as logic. The whole point of our work on Content` is to make it easy to intercept. We already "wrap" responses in include type logic in a very similar way.

Also, we would need to write Servlet specific HCFs for every possible HCF that needs to perform redirects/writes because we want to use Servlet APIs to do those, not core APIs. Currently, we have only one such extension point, the ResourceService.

I don't think so, as we can just wrap the servlet request/response as core request/responses and send them into the resource service so it will go via any servlet response wrappers. We already do this in other places (although it perhaps has not been generalized). But we need to avoid having multiple versions of any such behavior, else we will end up with core, ee9, ee10, ee11, ... etc. versions. Far better to have a single core implementation and use wrapping to access that from servlets.

Pseudo-code for file mapping:

class FileMappingHC extends HC.Wrapper {
  void process(Request req, Response res, Callback cbk) {
    if (couldMap == null) {
      buffer = tryMapping(getWrapped());
      couldMap = buffer != null; 
    }
    if (couldMap == Boolean.TRUE) {
      // TODO: must have a Servlet version for this.
      res.write(true, buffer.slice(), cbk);
    } else {
      getWrapped().process(req, res, cbk);
    }
  }
}

That's not how I see it at all. You would not wrap a 'HttpContentProcessor' with a 'FileMappedHttpContentProcessor' unless you already knew it could be mapped. So the implementation of process will be something like:

class FileMappingHCP extends HCP.Wrapper {
  void process(Request req, Response res, Callback cbk) {
    writeHeaders(res, getHttpContent());
    Response.write(req, res, _preMappedBuffer, cbk);

The only logic we'd put in process is logic that is dependent on something about the request and not on something about the resource. Decisions based on the resource would be made by the factory.
So for example, a PrecompressedHCP would select which of several contained HCP's it delegates to, based on the accept headers of the request.

[sbordet]

@gregw I thought you wanted HttpContent implements Request.Processor?

If you precompute the file mapping, the caching etc. now all the work is being done by the constructor at the HCF.getHttpContent(pathInContext) call.

I thought we did not want to do that to avoid as much as possible the stampede effect.

So if we are still on that idea of getHttpContent() be fast and light, and do all the rest in the second operator (be it getByteBuffer() or process(), then my pseudo implementation stands.

I just wanted to try to write it down to understand how good/bad was it.
Does not look that bad... it's a couple of comparisons after the initial load (which suffers from stampede -- but I guess the stampede can only be moved around).

[gregw]

@gregw I thought you wanted HttpContent implements Request.Processor?

No. read all my comments above where I proposed:

    interface HttpContentProcessor extends Request.Processor
    {
        HttpContent getHttpContent();
    }

This is server side only stuff, so it can be in o.e.j.server. If HttpContent is server only as well, then perhaps we move and combine. But either way, it is just not a key issue. So long as there is an entity that has both the HC and a Processor, then this approach works. Many ways to associated and I consider that an implementation detail.

If you precompute the file mapping, the caching etc. now all the work is being done by the constructor at the HCF.getHttpContent(pathInContext) call.

Not necessarily. It can be done that way, and probably should be for a start (optimize the fast path first!). But because we can asynchronously wait with process, there are many ways of actually solving stampede (as opposed to just moving it to 'getByteBuffer').

For example, the first request(s) to the factory could be just given the un-mapped HCP, whilst the mapped buffer is prepared and given to subsequent requests; or we could initially return a HCP that did the complexity and made process calls threadlessly wait, and then replaced itself in the factory once the buffer was available. I don't know if such complexity is warranted as I think mapping is quick, but there are plenty of alternatives. Either way, it would be good to come up with a solution that once the slow path was finished (i.e the initial request(s)) then the fast path was as simple as possible.

I thought we did not want to do that to avoid as much as possible the stampede effect.

Sure, but there are many ways to skin that cat. The getBuffer approach doesn't solve stampede problems, it just moves the stampeded from one spot to another!

So if we are still on that idea of getHttpContent() be fast and light, and do all the rest in the second operator (be it getByteBuffer() or process(), then my pseudo implementation stands.

No, because you can have a fast/light getHttpContent() that returns a sub-optimal HCP for the first request(s). Once the the optimal version is available, then it can be replace itself in the factory. Or there can be threadless waiting, or lots of other solutions.

The point being that if we want, the process approach can do exactly what the getByteBuffer approach does, or it can do more if we decide it is necessary. The converse is not true, as the getByteBuffer approach is unable to do everything that the process approach can, so it limits our flexibility and makes extension have to be done in the service rather than in HCF or HC.

I just wanted to try to write it down to understand how good/bad was it. Does not look that bad... it's a couple of comparisons after the initial load (which suffers from stampede -- but I guess the stampede can only be moved around).

Stampede can be solved if we have async waiting. Just not sure that is necessary for us to implement initially. I'm sold on the idea of let's optimize the fast path first and then see what slow paths turn up in the flame graphs before putting too much effort into them. So in that spirit, having a MappedHttpContentProcessor that just simply uses a pre-mapped buffer is optimizing the fast path. If that causes problems by taking too long in getHttpContent, then we can optimize that and there are plenty of options for doing so.

Also, I think you are somewhat confusing what the stampede problem is. If we get 1000s of initial requests for a resource, then stampede is not making 999 of them wait for the first. A stampede is if the 999 all create the same expensive resource that the first one does, only for them to be thrown away. So we can solve stampede by:

1. blocking in getHttpContent, which we have already decided is blocking anyway. This is simple but is kind of a global lock on all resources not just a specific resource.
2. blocking in HC.process, which is still blocking, but at least not a global lock. This is what is being done with getByteBuffer now.
3. providing a HC that delegates to it's wrapped HC until such time as the fast resource is ready. This assumes the HC will be cached... also it can make a mini stampede if the nested resource is itself expensive.
4. providing a HC that threadlessly waits for the expensive resource to be available and then applies it to all the waiting calls to process. This needs a queue of req/res/cb tupples, but it could easily be generalized and used in multiple places. I'm not sure we do this initially, but at least we have the option to do it if stampede proves to be a problem.

I think 1. allows for the best optimization of the fast path. If the global lock becomes a problem, then wrapping that fast HC in a generic impl of 3. or 4. should be easy to do and would trade off a virtual read plus extra delegation in the fast path for avoiding the global lock.

[gregw]

See also discussion at #8767 (comment)

[gregw]

@lachlan-roberts @sbordet @joakime @lorban I'm just re pitching the need for an async API in resources in this comment. It is a distillation of what I think after all the conversations above (and elsewhere):

One of the goals of the refactoring that has been done on the ResourceService in Jetty-12 is to move complexity out of the core service into optional components - primarily the HttpContentFactorys and wrappers of HttpContent. This has been done for features such as caching and file mapping, but it has not been done for features like welcome files, precompressed content and re-dispatching. These features have not been moved out because the API based on HttpContent#getByteBuffer is not powerful enough to allow them to be done.

The very first website we deployed on the new ResourceService required a mode of welcome file that we had not considered, so that in order to deploy we had to modify the ResourceService itself and add extra mode and configuration. This goes against the main goal of the refactor. There will be other exceptional cases we encounter with serving resources and we need an extensible API that will allows new behaviors to be added without extending the core service. A Request.Processor based API is sufficiently powerful to allow welcome files and similar features to be treated as extensions rather than as core features.

One such new behavior that has often been suggested is the use of a 3rd party caching library. However, we are currently unable to use any 3rd party HTTP caching library because the getByteBuffer style API does not give the caching component access to the response headers, thus it cannot inspect Cache-Control and other related meta data that is used to control the cache. A Request.Processor based API is sufficiently powerful to support 3rd party HTTP caches.

The HttpContent#getByteBuffer() API suffers from an ill-defined contract. It is unclear to the implementation of the method if it is being called as a once off just to serve the resource cheaply or if it is being called by a caching layer (or similar) that is prepared to invest in an expensive buffer because it will be reused. The implementation has a choice between returning: null to allow a stream based serving of the content; a buffer that will be GC'd after usage; a slice of a retained buffer that will be released when the HttpContent is; a slice of a File mapped buffer. But the implementation has no information available on which to base this decision, so we must rely on configuration to setup the content factories so that the "right" choice is made. But there may not be a single "right" choice! There have been frequent misunderstandings in discussions about HttpContent design because we don't know or can't agree on the right choice.

A Request.Processor based API does not use ByteBuffers as a contracted exchange, thus a HttpContent is clearly not responsible for deciding how the caller might store/reuse the content. A HttpContent is responsible only for serving the content in the most efficient way it knows how. It is the caller that can decide if it wishes to cache the content, or perhaps use alternative mechanism (e.g Path API) to optimize access to the content. A clear distinction between responsibilities will result in more consistent and easy to maintain code. Moreover, because the unit if transfer is a Content.Chunk it is inherently Retainable.

With the getByteBuffer style, there has been a lot of discussion about if the expensive operations should be factory, constructor or the getByteBuffer method itself, as this can effect blocking and stampede behavior. However, it turns out that because of the way HttpContents may be chained, then this distinction is rather pointless. If one factory moves the expensive operations into getByteBuffer to avoid contention and/or stampede on the factory, then this is negated if a cache is put in front, as that expense is now moved to the factory/constructor of the cache.

Finally, we have the async Request.Processor API as a core API in jetty-12. It is async, well known and well supported. It is certainly capable of serving resources. Why invent a new blocking API with unclear contract and the need to fallback to slow alternatives if null is returned? The current content factories / wrappers can be supported more or less exactly as they are now, but with the better API. I see no reason not to change to it.

@lachlan-roberts do you want to give this a go, or should I?

[gregw]

@lachlan-roberts can you link your branch here. Was there ever a PR that actually got reviewed? I think we need to capture the discussions around this.

[lachlan-roberts]

@gregw the branch is here https://github.com/eclipse/jetty.project/tree/jetty-12.0.x-AsyncHttpContent

It never had a PR but we reviewed in hangout with @sbordet and @lorban and decided that it introduced too much unnecessary complexity.

[gregw]

@gregw the branch is here https://github.com/eclipse/jetty.project/tree/jetty-12.0.x-AsyncHttpContent

It never had a PR but we reviewed in hangout with @sbordet and @lorban and decided that it introduced too much unnecessary complexity.

Yeah but I think it might also introduce some necessary correctness (i.e. #9079) !

[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]

In some ways this is related to #11094 where I'm adding async APIs to RetainableByteBuffer. A HttpContent is kind of similar

[gregw]

Building on the work in #11598 that makes RetainableByteBuffer a more generic buffer API, we should look at changing HttpContent to either have-a RetainableByteBuffer or extend RetainableByteByffer.

The key API would be RetainableByteBuffer.writeTo(Content.Sink sink, boolean last, Callback callback), which would allow content to be asynchronously flushed, even if it was not just a single buffer in memory.
The retainability would also be useful for caching, as a release would be done when it was removed from the cache, but that would not break any writes in progress that would have their own retention.

[lorban]

Unfortunately, you cannot have RetainableByteBuffer.writeTo(Content.Sink sink, boolean last, Callback callback) as RetainableByteBuffer is in jetty-util and Content.Sink is in jetty-io. This is the core reason that lead to the way things were done in #11364.

[gregw]

The API of HttpContent currently has methods including:

    long getContentLengthValue();
    ByteBuffer getByteBuffer();
    void release();

and the new RetainableByteBuffer has methods including:

     long size();
    ByteBuffer getByteBuffer() throws BufferOverflowException;
    boolean release();

So I think the simplest solution here is that we make HttpContent extend RetainableByteBuffer and then pretty much everything will continue to work with some trivial API adjustments... but we can then rewrite the ResourceService to use the void writeTo(Content.Sink sink, boolean last, Callback callback) method from RBB and we will be async!

[gregw]

@lorban I've had a quick start at the approach above (HttpContent extend RetainableByteBuffer) and that first part looks pretty trivial.
Do you want to do it, or shall I turn my hack into a branch?

[lorban]

@gregw Making HttpContent extend RetainableByteBuffer would mean that HttpContent.getByteBuffer() cannot return null, but this behavior is (so far) expected as HttpContent's byte buffer is meant to represent cached content that can be constructed from getResource().

How would you serve uncached 2+ GB files without keeping if (instanceof) checks?

[gregw]

@lorban you can have a getByteBuffer() that returns null (although technically it would be better for it to now throw BufferOverException), but the HttpContent type needs to implement the writeTo method. These implementations can use Content.copy and ByteChannelContentSource or InputStreamContentSource to keep the write async.

I've played around with it at have a few tests working but more work is needed. Have a look at the experiment/jetty-12.1.x/AsyncContentType branch. I'll clean it up fix some more test/types tomorrow.

[lorban]

@gregw There is no need for HttpContent to extend from RetainableByteBuffer for it to expose a writeTo(Content.Sink sink, Callback callback) method and to remove the getByteBuffer() method.

The IOResources helper I created should have been methods on Resource; we couldn't do that because of packaging. But there is no reason preventing us from adding copy(Sink, ..., Callback) (or writeTo(Sink, ..., Callback)) methods to HttpContent with signatures similar to what we have in IOResources.

If we were still in the old Jetty 11 model, I'd even consider that copy(Response, ..., Callback) would potentially be even better as the HttpContent instance could take care of the status and headers (given enough instructions in the ... parameters). We could cache 404 responses instead of always checking on disk that a file does not exist for instance.
But now that Jetty 12 has more than one response type, I'm not sure HttpContent is the right place to abstract that out.

[gregw]

@lorban but why isn't a HttpContent is-a RBB, just like a chunk is RBB? It has the same signature?

[lorban]

@gregw HttpContent.getByteBuffer() was kept around only for caching and mmap'ed files, to allow code looking like this:

        ByteBuffer buffer = httpContent.getByteBuffer();
        if (buffer != null)
            sendContent(buffer, callback);
        else
            sendContent(httpContent.getResource(), callback);

I checked getByteBuffer() is never called except immediately before writing the buffer if it's not null, with one exception: where we introduced RangeWriter... to have a writeTo() method.

I do not see a reason why HttpContent should expose its internal buffer if it has one, nor why it should be retainable. I even think we could get rid of release() on it and make it a simple, non-lifecycled class. If lifecycling is needed, it should be done at the factory level.

I'm going to try transposing my thoughts into code today to have a better basis for discussion.

[gregw]

@lorban I'm definitely not opposed to what you are describing. No getByteBuffer and no retain/release sounds good. That will make a HttpContent more like a Request.Handler... in fact it should just implement that, since passing in the request is useful for getting the bytebuffer pool if needed, and passing in last makes no sense!

Standing by to see your code and tools down on this PR now.

[gregw]

Note that I think a HttpContent is-a Request.Handler and not a Handler