FISH-464 Race conditions and stability

[dmatej]

This is a PR with commits going also into Eclipse's master (3.0.0-SNAPSHOT) backported also to Payara maintenance.

The release of 2.4.4.payara-p5 is not planned at this moment, but this branch was used to test changes with yesterday's state of Payara Server master and passed all available tests.

Current Payara 5.2020.7-SNAPSHOT with these commits in it's own patch of Grizzly passed following tests:

• all Grizzly's own tests
• all Payara's own tests
• TCK - websockets
• TCK - servlet
• my private test using arquillian and Apache HTTP Client and customized reproducer from Async servlet failed to send large data frames completely when HTTP/2 trailers are present eclipse-ee4j/grizzly#2016
• HTTP/2 Push reproducer "100 Delphines" (after reconfiguring Payara a bit to use 100 threads for HTTP or more)
  note - this test causes exceptions with Firefox (remote stream reset), but no exceptions on Chrome
• Reproducer from this issue HTTP/2 server push race conditions eclipse-ee4j/grizzly#2111 (PrimeFaces Showcase)

[dmatej]

How to test (not a script)

cd grizzly; 
mvn clean install;
cd payara;
mvn clean install -Dgrizzly.version=2.4.4.payara-p5-SNAPSHOT -Pfastest,QuickBuild
unzip 
asadmin start-domain
enable http2 push - Configurations - server-config - Network Config - Protocols - http-listener-2 - HTTP - check HTTP/2 Push Enabled and save 
boost capacity: Configurations - server-config - Thread Pools - http-thread-pool - set Max Thread Pool Size to 100 and save
asadmin restart-domain

Then you can try Delphines and also PrimeFaces reproducers.

[lprimak]

Looks good to me. I don't see any 'race conditions' fixed here though or am I missing something?
I do see exception thrown correctly when they were just ignored before

[dmatej]

It is not easy to explain, I just went step by step after exceptions in logs, failing tests, reproducers, etc.

Some "race condition" is still possible, but it is even aceptable by the spec, because whole system is so much asynchronous and multithreaded, that when something goes wrong (exceptions like NPE, buffer underflow/overflow, misformatted header, etc.), server is allowed to respond to the client by GOAWAY header and closing the stream OR simply by closing the stream/connection.
That is pretty hard to test - the h2spec test set usually has some set of "acceptable results" of the test, so often it is both "socket closed" and "GOAWAY". BUT! The system is multithreaded, so it is also possible that the cliend even already accepted some data from other streams.

So those race conditions are not that "internal JVM classic" like wrong locking and synchronization we originally expected, but rather issues with causality and correlation.

I'm still not sure if it is everything, but probably the best way how to find out is to release it - maybe at the start of the next year, so I would have time to revisit the Apache Client reproducer and check once again that there is no other trap - and if it is, fix it :-)
But that should be in another standalone PR as this one was heavily tested.