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

Cleanup of websocket in Jetty-12 #9233

Closed
lachlan-roberts opened this issue Jan 31, 2023 · 4 comments · Fixed by #9235 or #9234
Closed

Cleanup of websocket in Jetty-12 #9233

lachlan-roberts opened this issue Jan 31, 2023 · 4 comments · Fixed by #9235 or #9234
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@lachlan-roberts
Copy link
Contributor

  • resolve JPMS warnings for websocket in jetty-12

  • cleanup code

  • The common modules have private utility classes to be used by only the client/server modules, but also have some classes which need to be exposed publicly. What classes are and need to be exposed needs to be reviewed.

  • websocket-core common has a bunch of utilities and message sink classes which need to be exposed to about 20 modules, but are not made public. This is producing lots of JPMS warnings and needs to be reviewed.

@gregw
Copy link
Contributor

gregw commented Feb 1, 2023

I have some open source concern with the way that we have created internal common components. The websocket-common module is available to be used by multiple implementations of websocket (ee8, ee9, ee10 etc.). However because it has internal components we have to just through some JPMS hoops with a module-info.java like:

    // The Jetty & Jakarta API Layers need to access both access some internal utilities which we don't want to expose.
    exports org.eclipse.jetty.websocket.core.internal.util to
        org.eclipse.jetty.ee8.websocket.jetty.common,
        org.eclipse.jetty.ee8.websocket.jetty.client,
        org.eclipse.jetty.ee8.websocket.jetty.server,
        org.eclipse.jetty.ee8.websocket.javax.common,
        org.eclipse.jetty.ee8.websocket.javax.client,
        org.eclipse.jetty.ee8.websocket.javax.server,
        org.eclipse.jetty.ee9.websocket.jetty.common,
        org.eclipse.jetty.ee9.websocket.jetty.client,
        org.eclipse.jetty.ee9.websocket.jetty.server,
        org.eclipse.jetty.ee9.websocket.jakarta.common,
        org.eclipse.jetty.ee9.websocket.jakarta.client,
        org.eclipse.jetty.ee9.websocket.jakarta.server,
        org.eclipse.jetty.ee10.websocket.jetty.common,
        org.eclipse.jetty.ee10.websocket.jetty.client,
        org.eclipse.jetty.ee10.websocket.jetty.server,
        org.eclipse.jetty.ee10.websocket.jakarta.common,
        org.eclipse.jetty.ee10.websocket.jakarta.client,
        org.eclipse.jetty.ee10.websocket.jakarta.server;

So this is making these common components available ONLY to our own packages. This feels a bit restrictive for a project that prides itself on being open, flexible and extensible. Essentially what we are saying is that if somebody wanted to create their own app environment with websocket support, then they cannot use our common components to do so. Except that they can, by the terms of our license. It is just now that they will need to fork the project and modify at least the module-info so that they can use our common code to extend websocket in the way that they wish.

I understand the desire to flag these APIs as not fully public - ie we reserve the right to change them. But this feels a bit too restrictive to me. This is not the same as private methods, it is more akin to if there was a way to make protected methods that only we could use in other modules.

Why, for example, are our websocket Parser/Generator classes internal and exported only to our own implementations. Why couldn't some other project use our websocket parser without the need to fork our project?

@gregw
Copy link
Contributor

gregw commented Feb 1, 2023

I am somewhat concerned that white lists of export statements is against at least the spirit if not the letter of the "non-exclusive" aspects of our licenses.

Specifically the EPL says things like:

b) Subject to the terms of this Agreement, each Contributor hereby
grants Recipient a non-exclusive, worldwide, royalty-free patent
license under Licensed Patents to make, use, sell, offer to sell,
import and otherwise transfer the Contribution of such Contributor,
if any, in Source Code or other form.

Having a public artifact of org.eclipse.jetty.core.websocket.common that can only be used in some ways by specifically listed org.eclipse.jetty modules does not really satisfy "non-exclusive ... use ... in Source Code or other form". Perhaps I'm reading too much into it, but I would very much like to avoid whitelists in our exports.

@waynebeaton your thoughts?

@janbartel
Copy link
Contributor

The meaning of "internal" seems to be different to that commonly accepted by eg OSGi. A package named internal will never be exported by the manifest generator because it means that it is private to that module. In order to make websocket work in osgi we had to add this line to expose the internal package so that other jetty or even non-jetty modules could resolve an Import against it:

<Export-Package>*,org.eclipse.jetty.websocket.core.common.internal.*</Export-Package>

We should not have to be adding such explicit Export statements: we should review what we mean by internal and what visibility it should have.

@waynebeaton
Copy link

Having a public artifact of org.eclipse.jetty.core.websocket.common that can only be used in some ways by specifically listed org.eclipse.jetty modules does not really satisfy "non-exclusive ... use ... in Source Code or other form".

The license refers to the actual source code, not how it linked or how APIs are exposed. As long as somebody can get the source code and change it, you're basically good-to-go from a licensing point of view. If anybody can fork it, then it's non-exclusive.

Perhaps I'm reading too much into it, but I would very much like to avoid whitelists in our exports.

There may be good technical and philosophical reasons to limit exports, but its not a violation of the license in letter or spirit.

HTH

lachlan-roberts added a commit that referenced this issue Feb 3, 2023
…ver-jpms

Issue #9233 - Fix some JPMS issues for websocket-core
lachlan-roberts added a commit that referenced this issue Feb 9, 2023
…til-cleanup

Issue #9233 - cleanup of websocket-core-common util package
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
4 participants