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

Issue #4722 - remove websocket-servlet #4723

Merged
merged 11 commits into from
May 7, 2020
Merged

Issue #4722 - remove websocket-servlet #4723

merged 11 commits into from
May 7, 2020

Conversation

lachlan-roberts
Copy link
Contributor

Issue #4722

Removed the websocket-servlet layer which was resulting in exposing websocket-core classes.

  • WebSocketServlet and WebSocketServletFactory have been merged into JettyWebSocketServlet and JettyWebSocketServletFactory which were extending them previously.

  • The other classes including WebSocketUpgradeFilter and WebSocketMapping were moved into the new package org.eclipse.jetty.websocket.util.server of websocket-util, but maybe these should be in websocket-core instead

@lachlan-roberts
Issue #4722 - remove websocket-servlet
89c4504 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Issue #4722 - fix typo in JavaxWebSocketConfiguration
9298883 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

lachlan-roberts commented Apr 15, 2020 •
edited
Loading

I'm getting a failure on this branch for DistributionTests.testSimpleWebAppWithWebsocket saying

java.util.ServiceConfigurationError: org.slf4j.spi.SLF4JServiceProvider: Provider org.eclipse.jetty.logging.JettyLoggingServiceProvider not found

Any idea what would cause this @joakime @olamy , is this something to do with the recent logging changes?

@olamy
those jars are provided as they come from jetty distribution if not t…
5199e78 
…hey try to load slf4j impl

Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy
Copy link
Member

olamy commented Apr 15, 2020

@lachlan-roberts should be fixed with this commit 5199e78

joakime
joakime approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@lachlan-roberts
Issue #4722 - changes from review
213e157 
- Fix packages exposed in the websocket configuration
- Make servlet dependency for websocket-util optional

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
joakime
joakime approved these changes Apr 16, 2020
View reviewed changes
Copy link
Contributor

@joakime joakime left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still a 👍 from me

@lachlan-roberts
Issue #4722 - fix build, websocket util exporting jetty servlet with …
e189691 
…jpms

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

@joakime the change to make jetty-servlet an optional dependency for websocket-util has caused some problems.

The last commit got it working working but giving a JPMS warning as there are servlet classes in the public signature of websocket-util server package.

Warning:(106, 19) java: class org.eclipse.jetty.servlet.FilterHolder in module org.eclipse.jetty.servlet is not indirectly exported using requires transitive

As an alternative I could introduce another module which is shared only between both websocket-jetty-server and websocket-javax-server, called something like websocket-server-common and then move the WebSocketUpgradeFilter and other classes here. I still think WebSocketServlet does not belong in that package and the current changes to merge that with JettyWebSocketServlet is a good change.

@gregw
Copy link
Contributor

gregw commented Apr 20, 2020

@lachlan-roberts I'm loath to introduce more modules. I'm sure JPMS can deal with optional/provided dependencies.... ask @sbordet for advice or look for other examples of provided dependencies in the code base.

@lachlan-roberts
Copy link
Contributor Author

@gregw I asked @sbordet for advice and he said it would be best to introduce a new websocket-util-server module.

@lachlan-roberts
Issue #4722 - create new module websocket-util-server
b586978 
- This module contains the WebSocketUpgradeFilter.
- Also has an internal package with the components used
  to implement websocket upgrades common to both the
  jetty and javax websocket implementations.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

I pushed a commit to introduce a websocket-util-server module which has the WebSocketUpgradeFilter and other classes in internal package.

It only exports the internal package to websocket-jetty-server and websocket-javax-server and exports the base package (containing only the upgrade filter) to all.

@lachlan-roberts
Issue #4722 - fix typo for websocket-util-server jar
33a5db0 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@joakime
Copy link
Contributor

joakime commented Apr 22, 2020

The osgi tests on this branch are failing.

lachlan-roberts
lachlan-roberts commented Apr 30, 2020
View reviewed changes
jetty-osgi/pom.xml Outdated
@@ -90,7 +90,7 @@
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-websocket</artifactId>
<artifactId>jetty-websocket</artifactId> <!-- TODO: is this even right? -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@janbartel while debugging I saw this, any idea what this is for? I don't know what this is referring to, I don't think there is any jetty-websocket artifactId in jetty 10 so I thought this should be an errror.

@lachlan-roberts
Fix broken OSGI tests due to websocket-util-server
3a3c5e5 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Fix broken WebSocket UTF-8 continuation test
9bc9082 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-10.0.x' into jetty-10.0.x-…
e5cca92 
…WebSocketServlet

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts requested a review from gregw May 1, 2020 05:49
@lachlan-roberts
Copy link
Contributor Author

@gregw || @joakime can I get a tick after the new changes where I added the module jetty-util-server? I'd like to get this merged before we do another release of 10.

gregw
gregw approved these changes May 7, 2020
View reviewed changes
Copy link
Contributor

@gregw gregw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@lachlan-roberts lachlan-roberts merged commit 2e1c01a into jetty-10.0.x May 7, 2020
@lachlan-roberts lachlan-roberts deleted the jetty-10.0.x-WebSocketServlet branch May 7, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joakime joakime joakime approved these changes

@gregw gregw gregw approved these changes

@olamy olamy olamy 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.
4 participants
@lachlan-roberts @olamy @gregw @joakime