Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Aggregated fixes from Payara #2112

Merged
merged 26 commits into from
Nov 21, 2020
Merged

Aggregated fixes from Payara #2112

merged 26 commits into from
Nov 21, 2020

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Nov 19, 2020 •
edited
Loading

Fixes #2016 and #2111

This is a aggregated PR of Payara's fork of 2.4.4, I hope I didn't skip anything.
Current Payara 5.2020.7-SNAPSHOT with these commits in it's own patch of Grizzly passed following tests:

Merge:

@dmatej dmatej force-pushed the eclipse-2016-http2Trailers-clear branch from 1d495fb to 8ba9b17 Compare November 19, 2020 23:42
@dmatej
Copy link
Contributor Author

dmatej commented Nov 19, 2020

All commits are signed, but eclipsefdn/eca has different opinion - what can I do?

@edbratt
Copy link
Contributor

edbratt commented Nov 20, 2020

I agree, it does appear you have signed all the commits and you do have a valid ECA. A committer can go ahead and merge, once a review has been completed.

All commits are signed, but eclipsefdn/eca has different opinion - what can I do?

@dmatej dmatej force-pushed the eclipse-2016-http2Trailers-clear branch from 8ba9b17 to f5b7381 Compare November 20, 2020 06:38
@dmatej
Copy link
Contributor Author

dmatej commented Nov 20, 2020

I tried even to match my name including czech letters č and ě, but it is still the same. I'm giving up fighting with the eclipsefdn/eca checker ... the PR is prepared to be merged now, I don't plan any other pushes except those which would come from reviews.

dmatej and others added 22 commits November 20, 2020 14:59
@dmatej
Issue eclipse-ee4j#2016 Possibility to run tests with different JSSE …
254f80c 
…implementations

- default is none (uses JDK's impl)
- profile npn uses grizzly-npn-bootstrap
- profile openjsse uses openjsse
- versions can be overriden from command line (to compare results)

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Logging in tests
1775e35 
- important loggers are explicitly named in logging.properties

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed dependency artifactId
f4866af 
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Add support for JDK11 ALPN
a2f7f4c 
Create new phase in the handshake listener to allow registration of the
custom ALPN logic. This allows the HTTP/2 filter to work correctly.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Prevent HTTP/2 push when globally disabled
627b53f 
Streams still allowed pushing resources when globally disabled as the
stream itself and the session both allow it. This change makes the
stream also respect the global configuration.

Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed Tyrus dependency on HandshakeListener i…
7254635 
…nterface

- moving this interface out is a breaking change

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Added trailer to output queue to prevent earl…
7688da1 
…y connection closure.

Signed-off-by: Matt Gill <matthew.gill@live.co.uk>
Signed-off-by: David Matějček <dmatej@seznam.cz>
@MattGill98 @dmatej
Issue eclipse-ee4j#2016 Removed second check for trailer.
bfbf471 
Signed-off-by: Matt Gill <matthew.gill@live.co.uk>
Signed-off-by: David Matějček <dmatej@seznam.cz>
@MattGill98 @dmatej
Issue eclipse-ee4j#2016 Removed second deflator lock.
57c951f 
Signed-off-by: Matt Gill <matthew.gill@live.co.uk>
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed TrailersTest - missing lock
a90d6e9 
- partial cleanup, but requires much more than this
- DefaultOutputSink
  - combination of "fixes by refactoring" mixed with the changes in the flow
    of the logic
  - sendTrailers now locks the deflater
- added some javadocs and much more logs

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Better compatibility detection
fccd33d 
- can run with any compatible implementation, not only NPN Bootstrap
- with new JDK versions after 8u250 uses it's JSSE impl by default
- still can use older NPN bootstrap versions if configured
- can use also other implementations (openjsse)

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Better logging, reliable locking, variable na…
fef6f98 
…mes, but ...

- in this state grizzly:
  - fails TrailersTest (grizzly-http2; race condition, passes if I use stepping)
  - passes TCK tests with Payara (websocket)
  - still fails h2spec with Payara (race condition, different, but related issue)

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Using different port in tests
8d5a37f 
- 19001 is often occupied in Kubuntu

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed TrailersTest
1d35017 
- if the queue size is not same as reserved space, and if it is last content
  and the content is a trailer, send trailers
- if the queue size is same as reserved space, and record was already processed,
  don't send trailers.
- TrailersTest - just formatting and finals

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 CloseReason has toString now
17f8352 
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Minor syntax changes
5767b37 
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed maximum payload error.
8986bb8 
Signed-off-by: Matthew Gill <matthew.gill@live.co.uk>
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed NPE on missing CloseReason
2c531b4 
- original code was throwing NPE
- handler can be null
  - then we have nothing to handle the reason
- closeReason can be null
  - then we use "locally closed" as a default (may be incorrect)
- even ignored exception should be visible somewhere (logging)

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed IllegalStateException
488d5a6 
- based on h2spec results - client was able to cause ISE on the server simply
  by misformatted header
- assertions are worthless in production

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Escape both while loops if the record is null
716b945 
- fixes another NPE seen in logs

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Added logging for pruning streams
5b157a4 
Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 ALPN support test reenabled
360f431 
- Test results:
- OpenJDK 8u275, openjsse 1.1.5 ... SUCCESS
- OpenJDK 11.0.8, openjsse 1.1.5 ... FAIL ... as expected
- OpenJDK 8u275, npn bootstrap 1.9 ... FAIL ... as expected
- OpenJDK 8u232, npn bootstrap 1.9.payara-p1 ... SUCCESS
- OpenJDK 8u275 ... SUCCESS
- OpenJDK 11.0.8 ... SUCCESS

Signed-off-by: David Matějček <dmatej@seznam.cz>
@dmatej dmatej force-pushed the eclipse-2016-http2Trailers-clear branch from f5b7381 to 360f431 Compare November 20, 2020 14:45
@dmatej
Copy link
Contributor Author

dmatej commented Nov 20, 2020

I rebased the PR, the build will now fail here for two reasons:

  • npn is not released yet
  • I reenabled AlpnSupportTest, locally tested in following configurations (see npn/openjsse profiles, default is no profile -> using JDK's impl). Travis is using now obsoleted JDK8u151 which does not contain backported ALPN support from newer JDKs yet.
  • OpenJDK 8u275, openjsse 1.1.5 ... SUCCESS
  • OpenJDK 11.0.8, openjsse 1.1.5 ... FAIL ... as expected
  • OpenJDK 8u275, npn bootstrap 1.9 ... FAIL ... as expected
  • OpenJDK 8u232, npn bootstrap 1.9.payara-p1 ... SUCCESS
  • OpenJDK 8u275 ... SUCCESS
  • OpenJDK 11.0.8 ... SUCCESS

@dmatej dmatej mentioned this pull request Nov 20, 2020
Closed
@arjantijms
Copy link
Contributor

npn is not released yet

npn is now staged as 2.0.0, which can be used to do the build

@dmatej
Updated travis file
9b22457 
Signed-off-by: David Matejcek <dmatej@seznam.cz>
@dmatej
Issue eclipse-ee4j#2016 Fixed dependency version of NPN
28635f3 
Signed-off-by: David Matejcek <dmatej@seznam.cz>
@dmatej
Copy link
Contributor Author

dmatej commented Nov 21, 2020

The build passed locally with profile -Pstaging now, so after npn will make it to Maven Central it should pass now also on Travis :-)

@dmatej
Using current Ubuntu Focal (20.04) and openjdk8, fixed warnings
b5f23d1 
Signed-off-by: David Matejcek <dmatej@seznam.cz>
@dmatej dmatej force-pushed the eclipse-2016-http2Trailers-clear branch from 573ea71 to 24eab02 Compare November 21, 2020 12:54
@dmatej
Enabled staging repository
87569a5 
- required to be able to access staging repository with grizzly npn on Travis
- discussed with Arjan
- artifacts are not cached on Travis, so it should not cause any harm

Signed-off-by: David Matejcek <dmatej@seznam.cz>
@dmatej dmatej force-pushed the eclipse-2016-http2Trailers-clear branch from 24eab02 to 87569a5 Compare November 21, 2020 13:27
@arjantijms arjantijms merged commit a2ce777 into eclipse-ee4j:master Nov 21, 2020
@dmatej dmatej deleted the eclipse-2016-http2Trailers-clear branch November 25, 2020 19:22
@dmatej dmatej mentioned this pull request May 2, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arjantijms arjantijms arjantijms approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Async servlet failed to send large data frames completely when HTTP/2 trailers are present
4 participants
@dmatej @edbratt @arjantijms @MattGill98