TCK Challenge Tests with FailingHttpStatusCode exception

[breakponchito]

Specification:
cdi 4.0

Challenged test(s):
org.jboss.cdi.tck.tests.context.application.async.ApplicationContextAsyncListenerTest#testApplicationContextActiveOnError
org.jboss.cdi.tck.tests.context.conversation.determination.ConversationDeterminationTest#testConversationDetermination
org.jboss.cdi.tck.tests.context.request.async.RequestContextAsyncListenerTest#testRequestContextActiveOnError
org.jboss.cdi.tck.tests.context.session.async.SessionContextAsyncListenerTest#testSessionContextActiveOnError

TCK version:
4.0.5

Tested implementation:
Payara 6 (branch) with:

• WELD api: 5.0.Final
• WELD: 5.0.0.SP2
• CDI API: 4.0.1

Description:
The tests are failing because of the following exception thrown: com.gargoylesoftware.htmlunit.FailingHttpStatusCodeException: 500 Internal Server Error

those tests are throwing ServletException as part of the flow to test and the Payara Server side is setting the 500 error, but on the TCK side with the htmlunit framework those errors are Wrapped by a new exception the FailingHttpStatusCodeException. This is the wrong behavior and we don't want to hack the server side to return different codes.

We saw a way to solve this but it is on the TCK side by setting the following property to false: webClient.getOptions().setThrowExceptionOnFailingStatusCode(false)

With this value set to false all the failing tests will pass without hack the server side

Error Logs:
error-logs.txt

[manovotn]

This challenge is linked to a discussion on mailing list which started with this post - https://www.eclipse.org/lists/cdi-dev/msg00618.html

IIRC, for the sake of TCK test it doesn't even matter if the returned code of 200 or 500 (the code isn't checked at all).
Therefore, we could change it by adding the suggested option to web client and ignore the failure.

Whether this is right or wrong is a good question for someone from servlet spec I guess (does anyone know who to @mention here?). It is about an HTTP servlet supporting async and turning a request async with attached listener. The request, for some tests, results in an error that is going through yet another servlet.

Note that in the mailing thread, most participating people opted towards current TCK setup being correct from Servlet specification PoV. But also noted that the spec might need clarifications in this regard.

[arjantijms]

@markt-asf is the one to mention here, but he already answered the question in the email thread.

When a Servlet throws an exception during an async process, should the response be automatically set to 500, or is it the responsibility of the error handler to set the 500 (or other code). That's the main question.

Might also be good to compare what plain Tomcat is doing here, even though it doesn't run the CDI TCK; just create a similar situation with an async request and exception and check what gets send to the client.

[breakponchito]

@manovotn @arjantijms Hi guys, Do you have any update on this?. On our side we exclude the test from our arquillian tck configuration

[arjantijms]

@breakponchito did you ever check what Tomcat and/or Jetty are doing in this situation? It should be relatively easy to create a separate war with the same situation (without CDI) deploy that to Tomcat, and see what happens.

[breakponchito]

@arjantijms I'm going to review today, thanks. I'll let you know my findings

[breakponchito]

I already tested the servlet async call on tomcat throwing ServletException and it is showing 500 error

this means that the correct way to deal with async errors is to propagate to the client, is that ok? I think this challenge is valid. Who can help with this?

[manovotn]

I think this challenge is valid.

Since there is no reference to servlet spec which would imply the behavior, at least not from what I could find or what was mentioned here, I don't think you can claim the challenge valid.
We also have different behavior on several EE servers.

So I'd actually suggest to mark the challenge as invalid but change the test because whether it returns 200 or 500 is irrelevant for the CDI functionality we test. WDYT?

[breakponchito]

Do you mean to add the change to the following TCK tests?
org.jboss.cdi.tck.tests.context.application.async.ApplicationContextAsyncListenerTest#testApplicationContextActiveOnError
org.jboss.cdi.tck.tests.context.conversation.determination.ConversationDeterminationTest#testConversationDetermination
org.jboss.cdi.tck.tests.context.request.async.RequestContextAsyncListenerTest#testRequestContextActiveOnError
org.jboss.cdi.tck.tests.context.session.async.SessionContextAsyncListenerTest#testSessionContextActiveOnError

for all of them we just need to set the following property as false:
webClient.getOptions().setThrowExceptionOnFailingStatusCode(false)

[Pandrex247]

Since there is no reference to servlet spec which would imply the behavior

Is this not the perfect grounds for the challenge to be valid? The TCK mandates behaviour from the server which is not defined by any specification. It would surely fall under this point of the TCK Process for valid challenges:

Claims that a test asserts requirements over and above that of the specification

[manovotn]

for all of them we just need to set the following property as false:
webClient.getOptions().setThrowExceptionOnFailingStatusCode(false)

Yes, that's exactly what I meant.

Claims that a test asserts requirements over and above that of the specification

The test doesn't assert on return value of webClient.getPage(getPath(AsyncServlet.TEST_ERROR)); in any way.
The exception is a byproduct of this call, instead, CDI TCK tests that on error listeners are invoked which is defined and works.
Hence why I said I think it is invalid.

[manovotn]

I've added the invalid tag and approved the PR but I've also requested review from Ladislav to have a second pair of eyes on this.
Once merged, do you need the release out soon or is there no rush?

[Pandrex247]

I'm still unclear on how this is an invalid challenge.
It's not directly asserting on it, but the test requires that the server not return a failing status code (on a request that failed), and no specification defines that it must behave this way.

Since you're rejecting the challenge, and given that we can't pass the TCK unless we modify Payara specifically to satisfy this test, we would need a service release of the TCK ASAP please.

[manovotn]

It's not directly asserting on it, but the test requires that the server not return a failing status code

The test doesn't care what it returns, it makes no assertions on status code and has been like this for a long time.
The only reason why this now fails for some implementations is probably 0f9f0a4 which updated htmlunit version and they seem to have changed their default behavior. Hence we are now seeing this bug.

Since you're rejecting the challenge, and given that we can't pass the TCK unless we modify Payara specifically to satisfy this test, we would need a service release of the TCK ASAP please.

I am not sure why does it matter so much whether it is or isn't challenge but we can sure cut a release.
In fact any CDI committer (which @breakponchito seems to be?) can do that via Eclipse CI - https://ci.eclipse.org/cdi/
If you want me to do that, then I'll get to it tomorrow morning :)

[Pandrex247]

I think we're talking around each other now :P
The test cares what the server returns by nature of the update to htmlunit, which now throws an exception on a non-200 return code and thus fails the test (since the exception is not caught) - as you say it's essentially a bug in the test that only affects specific implementations 👍

It matters if the challenge is rejected because it means we can't exclude the test from runs of the TCK and still attempt to certify - we'd be stuck until a new version of the 4.0.x TCK was released with the merged fix.
If we're looking at doing a release of the TCK that quickly though then yes the problem goes away 😊

[breakponchito]

yes please, help us to do it

[manovotn]

4.0.6 should now be released and syncing with Central.