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.

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

Xbootclasspath removed from templates #25175

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Conversation

dmatej
Copy link
Contributor

@dmatej dmatej commented Oct 8, 2024

  • It was to allow older GF versions with JDK8 to use ALPN and NPN (which was never standardized).
  • Even later JDK8 versions have the support backported from JDK9.

Notes:

I created an issue for the library too, however it can stay there for years: eclipse-ee4j/grizzly-npn#21

Reproducer, which was failing when the change was not complete (just first commit + removed npn and npn-osgi dependencies):

mvn clean install -pl :grizzly-config -Dtest.logLevel=FINEST

- It was to allow older GF versions with JDK8 to use ALPN and NPN.
- Even later JDK8 versions have the support backported from JDK9.

Signed-off-by: David Matějček <[email protected]>
@dmatej dmatej added this to the 7.0.19 milestone Oct 8, 2024
@dmatej dmatej requested a review from a team October 8, 2024 14:03
@dmatej dmatej self-assigned this Oct 8, 2024
@OndroMih
Copy link
Contributor

OndroMih commented Oct 8, 2024

If we just remove grizzly-npn from the classpath, we get an exception from Grizzly if HTTP2 is enabled (which is by default):

java.lang.NoClassDefFoundError: org/glassfish/grizzly/npn/AlpnServerNegotiator
	at org.glassfish.grizzly.config.GenericGrizzlyListener.configureHttp2Support(GenericGrizzlyListener.java:744)
	at org.glassfish.grizzly.config.GenericGrizzlyListener.configureHttpProtocol(GenericGrizzlyListener.java:721)
	at com.sun.enterprise.v3.services.impl.GlassfishNetworkListener.configureHttpProtocol(GlassfishNetworkListener.java:188)

@dmatej
Copy link
Contributor Author

dmatej commented Oct 8, 2024

If we just remove grizzly-npn from the classpath, we get an exception from Grizzly if HTTP2 is enabled (which is by default):

java.lang.NoClassDefFoundError: org/glassfish/grizzly/npn/AlpnServerNegotiator
	at org.glassfish.grizzly.config.GenericGrizzlyListener.configureHttp2Support(GenericGrizzlyListener.java:744)
	at org.glassfish.grizzly.config.GenericGrizzlyListener.configureHttpProtocol(GenericGrizzlyListener.java:721)
	at com.sun.enterprise.v3.services.impl.GlassfishNetworkListener.configureHttpProtocol(GlassfishNetworkListener.java:188)

That is why the PR is just a draft, however together with my other changes it passed all tests. Which is bit suspicious :-)

@OndroMih
Copy link
Contributor

OndroMih commented Oct 8, 2024

It seems that the Grizzly NPN classes are still pulled by the GlassfishBootstrapClassLoader into the classpath. In fact, the Xbootclasspath was already removed from the templates from the server-config config, in 1d6cdb2. This PR only removes it from default-config.

I think the Grizzly NPN still must be on the classpath, otherwise the GlassFish Grizzly listener will throw the exception I mentioned when initializing HTTP2. We'll need to refactor the GlassFish listener to use the Java ALPN API instead of Grzilly NPN, and then we can remove Grizzly NPN dependency completely, from GlassfishBootstrapClassLoader, from GlassFish Embedded, and from the static shell manifest.

- however it is still required by grizzly-http2's Http2AddOn, so I added it
  to the nucleus-grizzly-all.

Signed-off-by: David Matějček <[email protected]>
@dmatej
Copy link
Contributor Author

dmatej commented Oct 8, 2024

Not exactly, the point is that grizzly-http2 uses the AlpnServerNegotiator interface, which just must be available. No need to refactor anything now, I just had to add this to appropriate place.
The JDK uses BiFunction nstead of this interface and by default it is also used, see log:

org.glassfish.grizzly.http2.AlpnSupport.<clinit> Detected ALPN compatibility info: org.glassfish.grizzly.http2.AplnExtensionCompatibility@4a3329b9ALPN available: true, ALPN is Grizzly: false, setHandshakeApplicationProtocolSelector in API: true, setHandshakeApplicationProtocolSelector in impl: true

Meaning:

  • ALPN available: true = we have SOME implementation supporting ALPN
    • JDK: sun.security.ssl.SSLEngineImpl and the required method is implemented:
      public synchronized void setHandshakeApplicationProtocolSelector(
          BiFunction<SSLEngine, List<String>, String> selector)  {
          conContext.sslConfig.engineAPSelector = selector;
      }
      
  • ALPN is Grizzly: false = class sun.security.ssl.GrizzlyNPN was NOT found on bootstrap classpath
  • The remaining two see this logic:
              if (isHandshakeSetterInApi()) {
                 engineClass = SSLEngine.class;
             } else {
                 engineClass = engine.getClass();
             }

However you are basically right that Grizzly doesn't need it any more, now it uses reflection to call the setter when it could call it directly. Even with latest JDK8.

@dmatej dmatej marked this pull request as ready for review October 8, 2024 20:54
@dmatej dmatej merged commit 7210ba6 into eclipse-ee4j:master Oct 9, 2024
2 checks passed
@dmatej dmatej deleted the npn-removal branch October 9, 2024 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants