Issue #12505 - Server to Servlet Error Handling

[janbartel]

Fixes #12505

Addresses:

• difference between jetty core error attributes and servlet api attributes
• handling error generated in core before servlet context is selected

[janbartel]

A couple of things

[gregw]

I approve, but then I have worked on this, so my review is not enough

[janbartel]

I approve! But I can't officially as I've created the PR.

[lorban]

Just found a few nits.

[lorban]

You could call getRequest and cast it to HttpServletRequest instead of introducing the _httpServletRequest field.

[lachlan-roberts]

Its also possible to directly do a sendError directly here, which might be more clear, but might require some tweaks to the ServletChannelState to handle sendError from IDLE.

[janbartel]

I don't think it's any clearer - at this point we have a core Response, not a HttpServletResponse.

[lachlan-roberts]

We do already know here it is a ServletContextRequest so we have access to the HttpServletResponse (checked directly above at the start of this method).

So you could do this servletContextRequest.getServletContextResponse().getServletRequestState().sendError(404, null);.

Currently this needs to set a special attribute to trigger calling sendError internally, when the call to sendError could be done right here.

[janbartel]

Well, we got into trouble last time by trying to handle the error here (with a Response.writeError that bypassed the ServletChannel etc), rather than letting the normal mechanism take care of it. IMHO that call chain isn't particularly clear either - (also, side note, that method is badly named, if it returns a ServletChannelState it shouldn't be called getServletRequestState).

@gregw what's your take on this? Want to try and take the short cut to sendError or set the attribute and flow through the normal execution path?

[gregw]

@lachlan-roberts At this point we have not entered the ServletHandler, so the ServletChannel state machine has not started. Thus I'm not sure that calling sendError here is exactly what we want to do. It might work, but it might also do other stuff that is not necessary. It is more likely to throw an ISE because the ServletChannelState will be IDLE.

The idea of this PR is to signal that a call to sendError is necessary, but to delay that until we are within the ServletChannelState machine.

[lachlan-roberts]

It would take only 3-4 lines to be changed in ServletChannelState to make it work.

I would just prefer use of API, either sendError or something else, because someone working on a similar case in future will not know they have to set both the response status and this attribute to trigger a sendError if it hasn't entered the SerlvetChannel yet.

[gregw]

@lachlan-roberts I'm the other way. I like the ISE being thrown if sendError is being called outside of the scope it should be. If we want to cause a send error on entry to the ServletHandler, it is best to use a different mechanism.
In this case the mechanism is to have both the response status set AND the error attribute set. It is done this way because checking attributes is a little expensive, so we first check the status. But unfortunately checking the status is not enough as there are some strange circumstances where a status is spuriously set before arriving at the ServletHandler. So if it is non-zero, we then check the attribute to be sure an error dispatch was intended.

@janbartel perhaps we should put a comment to explain that?

[gregw]

Nice