jetty-12 ee10 ServletRequestListeners called too many times on sendError

[janbartel]

jetty-12

Servlet tck error: https://jenkins.webtide.net/job/tck/job/tck-servlet-arquillian-experiment/job/jetty-12-ee10/119/testReport/junit/com.sun.ts.tests.servlet.spec.srlistener/URLClient/error/

The test sends a request to a servlet that does a sendError(403), and then sends another request to check how many times ServletRequestListener.requestInitialized and ServletRequestListener.requestDestroyed have been called. The test is expecting 3 calls: an initialize and destroy for the first request that sends the error, and then another initialize for the request that is checking the number of calls. The problem is that instead of 3 calls, we do 5: there is an extra initialize and destroy pair present before the last initialize.

The problem appears to be that ServletChannel.dispatch always calls ServletContextHandler.requestInitialized. This means that the first handle loop will call dispatch (which calls requestInitialized), but then the second handle loop for the SEND_ERROR case also calls dispatch, leading to the second call to ServletContextHandler.requestInitialized.

See line 656 of ServletChannel.dispatch.

Note: this may potentially affect ee9 too?

[janbartel]

Here's the relevant test failure output:

[main] ERROR org.eclipse.jetty.tck.AppTest - Test: 'checkLogSimple' failed.
java.lang.Exception:  Test Test Case Unable to find the following search string in the server's response: 
'Test PASSED'
 at index: 0
 Server's response:
-------------------------------------------
RequestListener is invoked/deleted more or less times than requiredExpect 3 times, got 5.Got expected value: in requestInitialized method of listener at position 0Got expected value: in requestDestroyed method of listener at position 1Got expected value: in requestInitialized method of listener at position 2Extra access to Listener: in requestDestroyed method of listener at position 3Extra access to Listener: in requestInitialized method of listener at position 4Test FAILED

-------------------------------------------

[janbartel]

The fix for this will also most likely fix:

com/sun/ts/tests/servlet/spec/srlistener/URLClient.java.forwarderror
com/sun/ts/tests/servlet/spec/srlistener/URLClient.java.includeerror
com/sun/ts/tests/servlet/spec/srlistener/URLClient.java.simpleasyncerror

[gregw]

@lachlan-roberts I was going to raise a TCK challenge, but then I realised that there is an issue with the difference between dispatching to the ErrorHandler and to an error page used by the ErrorHandler.

In the "error" test, sendError is called, we return from the servlet and then we need to generate an error page without a dispatch. We do this by dispatching to the ErrorHandler, so currently we are calling listeners when we shouldn't. But if the ErrorHandler has an error page, then it does an Dispatcher.error dispatch and at that point we should call the request listeners.

So I do think we need to change a little bit, but not in the way we discussed before.

[lachlan-roberts]

We have fixed this in ee10 Jetty-12 with #9677, would be good to confirm that these TCK tests are now passing.

[janbartel]

Fixed