WebSocketCoreSession leaks into JPMS exported classes

[sbordet]

WebSocketCoreSession is in an internal package that is not exported by its JPMS module.

However, Extension and AbstractExtension, which are JPMS exported classes, have WebSocketCoreSession in method signatures, which means that it needs to be visible.

These classes need to be restructured so that they have the proper JPMS visibility.

[sbordet]

@gregw @lachlan-roberts it's not clear to me if you want websocket-core to be invisible if people wants to use either Javax WebSocket or Jetty WebSocket.
Should application just e.g. requires org.eclipse.jetty.websocket.client, or also requires org.eclipse.jetty.websocket.core?

If so, then Jetty WebSocket DispatchedMessageSink and subclasses leak core's Frame class.
JettyExtensionConfig leaks core's ExtensionConfig class.
JavaxWebSocketContainer leaks core's WebSocketExtensionRegistry.
There are other examples of this, can be detected compiling in the IDE with -Xlint:exports

[gregw]

@sbordet @lachlan-roberts @joakime the intention is for core to be hidden from users of the javax and jetty websocket API packages.

The problem is extensions, as they have not been clearly put into either the implementation nor API camps. I believe that the implementation of extensions should be an implementation matter and not exposed to regular users of the APIs. API users may select and/or configure extensions - but should do so using only native based APIs rather dependencies on core.

However, some users will want to write their own extensions, in which case they should be required to have an explicit dependency on core.

Also, it may be that some APIs might define their own extension APIs (dumb idea but they might), in which case those APIs need to be implemented using core, but not exposing core.

[sbordet]

@gregw the problem is not only extensions. Core classes are leaked in signatures of major Jetty WebSocket classes.

A user that wants to use Jetty WebSocket classes embedded with JPMS will have to requires org.eclipse.jetty.websocket.core in its JPMS module.

That may be perfectly fine.

However, if we want to keep core hidden to Jetty WebSocket users, then we should avoid leaking of core classes.

If, instead, we don't want to hide core classes, then this issue can be closed without doing anything.

[gregw]

We want to hide core. It's implementation not API.

…

On Mon, 21 Oct 2019, 08:16 Simone Bordet, ***@***.***> wrote: @gregw <https://github.com/gregw> the problem is not only extensions. Core classes are leaked in signatures of major Jetty WebSocket classes. A user that wants to use Jetty WebSocket classes embedded with JPMS will have to requires org.eclipse.jetty.websocket.core in its JPMS module. That may be perfectly fine. However, if we want to keep core hidden to Jetty WebSocket users, then we should avoid leaking of core classes. If, instead, we don't want to hide core classes, then this issue can be closed without doing anything. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4226>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAARJLKO4TVUY3J32BLYD7DQPTDELANCNFSM4JCVWVPA> .