QACI-406 Websockets tck failure fix: Grizzly #24

dmatej · 2020-09-07T09:06:38Z

Fixes two websocket tests in TCK tests for 5.2020.5-SNAPSHOT
Still fails one of two TrailersTests - originally failed two.
More robust compatibility with JDK8 versions.
build finally does not depend on QACI-406 NPN compilable by JDK8u265 patched-src-grizzly-npn#4 - grizzly npn release can be postponed.

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

- important loggers are explicitly named in logging.properties

- moving this interface out is a breaking change

- 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)

dmatej · 2020-09-07T09:09:28Z

modules/grizzly/src/main/java/org/glassfish/grizzly/filterchain/DefaultFilterChain.java

@@ -253,14 +253,14 @@ protected NextAction executeFilter(final FilterExecutor executor,
        NextAction nextNextAction;
        do {
            if (LOGGER.isLoggable(Level.FINEST)) {
-                LOGGER.log(Level.FINE, "Execute filter. filter={0} context={1}",
+                LOGGER.log(Level.FINE, "before filter execution. filter={0} context={1}",


Symmetric logging is easier to find/grep and to understand

dmatej · 2020-09-07T09:09:58Z

modules/bundles/http/pom.xml

@@ -171,7 +171,7 @@
        <dependency>
            <groupId>org.glassfish.grizzly</groupId>
            <artifactId>grizzly-npn-api</artifactId>
-            <version>${grizzly.alpn.version}</version>
+            <version>${grizzly.npn.api.version}</version>


At the start I wasn't sure which property is for which dependency.

dmatej · 2020-09-07T09:10:59Z

modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/HandshakeListener.java

@@ -1,12 +0,0 @@
-package org.glassfish.grizzly.ssl;


Cause of the TCK failure. Tyrus depends on this interface, so it must not be moved outside the SSLBaseFilter class. Breaking change.

dmatej · 2020-09-07T09:11:23Z

modules/grizzly/src/main/java/org/glassfish/grizzly/ssl/SSLBaseFilter.java

@@ -1208,4 +1202,15 @@ public Buffer clone(final Connection connection,
            return originalMessage;
        }
    }
+


Moved back.

dmatej · 2020-09-07T09:12:10Z

modules/http/src/main/java/org/glassfish/grizzly/http/HttpTrailer.java

@@ -36,7 +36,7 @@
     * @return <tt>true</tt> if passed {@link HttpContent} is a <tt>HttpTrailder</tt>.
     */
    public static boolean isTrailer(HttpContent httpContent) {
-        return HttpTrailer.class.isAssignableFrom(httpContent.getClass());
+        return httpContent != null && HttpTrailer.class.isAssignableFrom(httpContent.getClass());


More robust. Possible NPE fixed.

dmatej · 2020-09-07T09:15:02Z

modules/http2/pom.xml

    </dependencies>
+
+    <profiles>


Possible to use alternatives, but default is JDK version, not npn.
It is not even a breaking change, because the bootstrap was created to fill the gap for JDK8 versions without ALPN support and it is an old copy of Sun jsse classes, which were enhanced and backported back to JDK8 from JDK9/11.

dmatej · 2020-09-07T09:19:16Z

modules/http2/src/main/java/org/glassfish/grizzly/http2/AlpnSupport.java

-    private static final Method nativeHandshakeMethod;
-
-    static {
-        boolean isExtensionFound = false;


When I started to test combinations of JDK vs. grizzly vs. npn-bootstrap vs. npn-api, AlpnSupport sometimes threw exceptions on unexpected places. See Confluence page for few details.
Finally, I created a new class for all this heuristic and tested it against all JDK8 versions I have (both AdoptJDK and Zulu 8u265 are the newest)

dmatej · 2020-09-07T09:52:33Z

modules/http2/src/main/java/org/glassfish/grizzly/http2/DefaultOutputSink.java

@@ -25,7 +25,6 @@
 import java.util.List;


Grizzly's tests were failing because of this class. I was able to fix one of two failing tests, but then I noticed that the lock usages are not always correct and is hard to find all paths. So I did around 100 small iterations to make the code more readable and to fix both tests.

dmatej · 2020-09-07T09:59:25Z

modules/http2/src/test/java/org/glassfish/grizzly/http2/AbstractHttp2Test.java

 * @author Alexey Stashok
 */
 public abstract class AbstractHttp2Test {
+    static {


Logging in tests, so you can see order of method executions in multithreaded tests.

pom.xml

MattGill98

Have you tested for regressions against PAYARA-3031?

dmatej · 2020-09-10T18:14:45Z

Have you tested for regressions against PAYARA-3031?

Nope, but I will add the reproducer to Payara Samples now. I have seen your 2 years old fix now, now I'm curious if it still works or not. But those locks before both our changes were used incorrectly.

- in this state grizzly: - still fails TrailersTest (grizzly-http2) - passes TCK tests (websocket) (THIS CHANGED!) - fails h2spec (race condition) - fails LargeResponseTest (Payara Samples - http2, race condition) - see lines with FIXME: this causes data corruption!

- fixed ALPN support - fixed two failing tests (trailers) - better compatibility and logging

SNAPSHOT version missed in version update commit

MattGill98 · 2020-09-23T14:40:15Z

I've pushed a minor update to the bom version - worth remembering in future that the mvn versions:set command needs running in the top level folder as well as the BOM folder

MattGill98

Looks like a good change - make sure to push upstream when you get a chance

dmatej added 6 commits September 2, 2020 12:10

QACI-406 Possibility to run tests with different JSSE implementations

d48b862

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

QACI-406 Logging in tests

8a0ae64

- important loggers are explicitly named in logging.properties

QACI-406 Fixed dependency artifactId

cdbf50f

QACI-406 Fixed Tyrus dependency on HandshakeListener interface

d8f7915

- moving this interface out is a breaking change

QACI-406 Fixed TrailersTest - missing lock

dbc3f1a

QACI-406 Better compatibility detection

09c8e3e

- 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)

dmatej self-assigned this Sep 7, 2020

dmatej commented Sep 7, 2020

View reviewed changes

QACI-406 Removed incorrect FIXME

255b55a

dmatej commented Sep 7, 2020

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

dmatej marked this pull request as ready for review September 7, 2020 16:49

dmatej requested a review from MattGill98 September 7, 2020 16:49

dmatej changed the title ~~QACI-406 Websockets tck failure fix~~ QACI-406 Websockets tck failure fix: NPN Sep 7, 2020

dmatej changed the title ~~QACI-406 Websockets tck failure fix: NPN~~ QACI-406 Websockets tck failure fix: Grizzly Sep 7, 2020

dmatej requested a review from MeroRai September 7, 2020 16:52

dmatej mentioned this pull request Sep 7, 2020

QACI-406 fixed tck websockets payara/Payara#4876

Merged

MattGill98 suggested changes Sep 10, 2020

View reviewed changes

dmatej added the DO NOT MERGE label Sep 11, 2020

dmatej added 2 commits September 22, 2020 12:09

QACI-406 Released grizzly 2.4.4.payara-p4

bb55a72

- fixed ALPN support - fixed two failing tests (trailers) - better compatibility and logging

dmatej force-pushed the QACI-406-websockets-tck-failure-fix branch from 566f2e8 to bb55a72 Compare September 22, 2020 15:10

dmatej mentioned this pull request Sep 22, 2020

QACI-406 Released grizzly 2.4.4.payara-p4 payara/Payara_PatchedProjects#327

Merged

dmatej requested a review from MattGill98 September 22, 2020 15:35

dmatej removed the DO NOT MERGE label Sep 22, 2020

Remove SNAPSHOT BOM dependency

3a956e5

SNAPSHOT version missed in version update commit

MattGill98 mentioned this pull request Sep 23, 2020

Correct Grizzly 2.4.4.payara-p4 bom versions payara/Payara_PatchedProjects#334

Merged

MattGill98 approved these changes Sep 23, 2020

View reviewed changes

MattGill98 merged commit 8a9d506 into 2.4.4.payara-maintenance Sep 23, 2020

dmatej deleted the QACI-406-websockets-tck-failure-fix branch November 19, 2020 07:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

QACI-406 Websockets tck failure fix: Grizzly #24

QACI-406 Websockets tck failure fix: Grizzly #24

dmatej commented Sep 7, 2020 •

edited

Loading

dmatej Sep 7, 2020

dmatej Sep 7, 2020

dmatej Sep 7, 2020 •

edited

Loading

dmatej Sep 7, 2020

dmatej Sep 7, 2020

dmatej Sep 7, 2020 •

edited

Loading

dmatej Sep 7, 2020

dmatej Sep 7, 2020

dmatej Sep 7, 2020

MattGill98 left a comment

dmatej commented Sep 10, 2020

MattGill98 commented Sep 23, 2020

MattGill98 left a comment

QACI-406 Websockets tck failure fix: Grizzly #24

QACI-406 Websockets tck failure fix: Grizzly #24

Conversation

dmatej commented Sep 7, 2020 • edited Loading

dmatej Sep 7, 2020

Choose a reason for hiding this comment

dmatej Sep 7, 2020

Choose a reason for hiding this comment

dmatej Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

dmatej Sep 7, 2020

Choose a reason for hiding this comment

dmatej Sep 7, 2020

Choose a reason for hiding this comment

dmatej Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

dmatej Sep 7, 2020

Choose a reason for hiding this comment

dmatej Sep 7, 2020

Choose a reason for hiding this comment

dmatej Sep 7, 2020

Choose a reason for hiding this comment

MattGill98 left a comment

Choose a reason for hiding this comment

dmatej commented Sep 10, 2020

MattGill98 commented Sep 23, 2020

MattGill98 left a comment

Choose a reason for hiding this comment

dmatej commented Sep 7, 2020 •

edited

Loading

dmatej Sep 7, 2020 •

edited

Loading

dmatej Sep 7, 2020 •

edited

Loading