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

Upgrade to jetty 9.3.34 to solve Blocking IllegalStateException with jetty 9.3.33+ in combation with pax-web 7.3.9 [PAXWEB-1274] #1553

Closed
ops4j-issues opened this issue Nov 5, 2020 · 14 comments
Labels
Milestone

Comments

@ops4j-issues
Copy link

Tom De Wolf created PAXWEB-1274

In later jetty versions a change was made that no longer allows the classloader to be set on the jetty server when it is already started. Pax-web currently still changes the classloader, even if the server was already started. This results in an IllegalStateException.

We bumped into this problem when trying to use jetty 9.4.33.v20201020 in combination with pax-web 7.3.9.

java.lang.IllegalStateException: STARTED
	at org.eclipse.jetty.server.handler.ContextHandler.setClassLoader(ContextHandler.java:1645) ~[?:?]
	at org.ops4j.pax.web.service.jetty.internal.JettyServerImpl$1.start(JettyServerImpl.java:325) ~[?:?]
	at org.ops4j.pax.web.service.internal.HttpServiceStarted.registerFilter(HttpServiceStarted.java:631) ~[?:?]
	at org.ops4j.pax.web.service.internal.HttpServiceStarted.registerFilter(HttpServiceStarted.java:603) ~[?:?]
	at org.ops4j.pax.web.service.internal.HttpServiceProxy.registerFilter(HttpServiceProxy.java:212) ~[?:?]

I refer to this line of code:

Above this line of code the class loader of Jetty is set.

Below this line jetty is started if not yet started.

Because jetty in a more recent version does not allow to set the classloader anymore if the server is already started the lines above line 326 doing that should be moved inside the if statement. Then this is only done when the server is not yet started.


Affects: 7.3.9
Fixed in: 8.0.0, 7.3.10, 7.2.20
Votes: 0, Watches: 4

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Achim Nierbeck Should I just create a pull request on github for this in order to get it fixed? And how can it be merged/released? Do I have rights to do this or can I get rights to do this/contribute?

Thx

@ops4j-issues
Copy link
Author

Achim Nierbeck commented

Yes please start with a pull request and if you’re already part of the “team” on GitHub you can merge yourself. Otherwise I’ll add you to the organisation.
If you would like that others will review it, then mention them as reviewers ��

@ops4j-issues
Copy link
Author

Tom De Wolf commented

After upgrading pax-web to the later jetty version 9.4.33.v20201020 the following integration test failed, and as such reproducing the problem:

[ERROR] Tests run: 2, Failures: 0, Errors: 1, Skipped: 0, Time elapsed: 8.444 s <<< FAILURE! - in org.ops4j.pax.web.itest.jetty.FilterIntegrationTest
[ERROR] testSimpleFilter(org.ops4j.pax.web.itest.jetty.FilterIntegrationTest) Time elapsed: 4.89 s <<< ERROR!
java.lang.IllegalStateException: STARTED

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Bumped into a NPE bug in jetty 9.4.33 which was already fixed in 9.4.34.v20201102 (jetty/jetty.project#5556 ). So upgrading to 9.4.34

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Achim Nierbeck I created this pull request

#293

does it look ok? it upgrades to the latest jetty version for pax-web 7.3.x

Next steps would be to

  • get this merged
  • get a 7.3.10 release including this (which would deblock users of pax-web to upgrade to the latest jetty which is important due to vulnerabilities in the earlier versions)
  • merge the changes in the main master branch

Can I do this? Who can help on this?

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Achim Nierbeck can you make me part of the team on github so I can merge myself as you mentioned?

can I also make a release then?

Thx

@ops4j-issues
Copy link
Author

Achim Nierbeck commented

Hi, made you part of the team.
Therfore free to merge yourself.
You could also create a release, in that case I would first check with Grzegorz Grzybek if he has any pending changes that need to go with the next release.

@ops4j-issues
Copy link
Author

Grzegorz Grzybek commented

In master-improvements branch (real 8.0.0-SNAPSHOT), I've approached classloaders differently - more in line with JavaEE, where web context's (javax.servlet.ServletContext implementation) classloader is set explicitly only during creation, not when it's started.

Tom De Wolf so for now you don't have to merge to master, as this branch will be replaced at some point (I did about 80% of the 8.0.0 refactoring) with current master-improvements branch.

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Achim Nierbeck are there specific release instructions somewhere? Then I can release 7.3.10 by following the correct steps.

@ops4j-issues
Copy link
Author

Jean-Baptiste Onofre commented

I’m doing the release (I have another issues to fix in progress). Some please, let me do it.

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Jean-Baptiste Onofre when do you expect to finish these issues? The problem is that I need a release with my fix to deblock a client.

Would it be a problem to release 7.3.10 now and push the other issues in a new 7.3.11?

@ops4j-issues
Copy link
Author

Jean-Baptiste Onofre commented

I plan to cut the release this morning.

@ops4j-issues
Copy link
Author

Jean-Baptiste Onofre commented

Please, again, let me cut 7.3.10 !

@ops4j-issues
Copy link
Author

Tom De Wolf commented

Jean-Baptiste Onofre no problem, that is fast enough. Thx.

@ops4j-issues ops4j-issues added this to the 8.0.0 milestone Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant