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

Support Apache Tomcat 10 #144

Closed
acestars opened this issue Mar 18, 2020 · 9 comments · Fixed by #159
Closed

Support Apache Tomcat 10 #144

acestars opened this issue Mar 18, 2020 · 9 comments · Fixed by #159
Assignees

Comments

@acestars
Copy link

Is there any plan to support apache tomcat 10? Because Apache Tomcat 10 has different dependencies.

@boris-petrov
Copy link
Member

@acestars - yes, support for Tomcat 10 is most definitely on the roadmap. Pull-request are welcome.

Can you share a link about these "different dependencies" that you mention?

@acestars
Copy link
Author

@boris-petrov you can read at https://tomcat.apache.org/migration-10.html. The packages required by Tomcat 10 changed from javax.* to jakarta.*

@f4lco
Copy link
Collaborator

f4lco commented Apr 27, 2020

I produced a (failed) proof-of-concept in my branch feature/tomcat-ten. The build is passing, however, there are some caveats. I left a couple of // FIXME comments.

It was a bit disappointing because trying to support legacy APIs in the "javax" namespace parallel to Jakarta introduced a mess. Rather than having a ready implementation I worked out areas in which Gretty requires more work (for supporting Tomcat 10, and Jetty 10).

To my knowledge, a compatibility layer does not exist, which would help with the transition. As I understand, this is generally a good thing: we must get rid of the "javax" trademark - which has to happen sometime - and migration aids would simply delay the change. It's a breaking one, but for most projects far from impossible. In return we get APIs which can evolve.

For Gretty, this means a number of modules / components are replicated (copied) in the current proof of concept with changed import statements:

  • the whole gretty-filter module
  • all glue regarding custom JAR scanners
  • in theory all integration tests

Because this list only speaks for Tomcat, it is likely incomplete. The last point - integration tests - is by far the heaviest. All integration tests would have to be replicated to use the new imports. This is why my PoC only contains a single, copied "hello world" integration test for Jakarta. So the current Tomcat 10 server container adapter is covered by only 1 out of more than 30 test cases.

Out of this experience I make the following proposal. The scope of supporting Jakarta-enabled servlet containers such as Tomcat 10 extends to:

  1. Making Gretty 3.x something like a "long term support" version with a dedicated branch which receives bugfixes for all servlet containers using the "javax" namespace
  2. Launching Gretty 4.x in master, which will only support the "jakarta" namespace. That release would rip out support for all existing servlet containers except Tomcat 10, and Jetty 10, as soon as released.
  3. The majority of work would probably go into integration tests. They need to be enabled one-by-one to catch regressions with Tomcat 10.

I think if none of the apps deployed from Gretty can use e.g. Tomcat 9 and 10 in parallel, there is no point in having a version of Gretty which can support both at the same time.
This has been an interesting journey. I'd be glad to hear your opinion on the matter, @boris-petrov.

@boris-petrov
Copy link
Member

@f4lco - thanks a lot for your time and the work you've done! Here's my 5 cents.

I think what you suggest makes perfect sense. I would make it the other way around though - make a branch for the "jakarta" things and leave master to use the "javax" stuff for now because Tomcat 10 is still in alpha - but only if that's not going to change soon - i.e. if Tomcat 10 comes out in a few weeks then yes, we can go as you said. But otherwise no need to have in master something that we won't release soon.

I'm not sure I understand why point 3 is in your list - isn't the "work" there just renaming imports? Theoretically everything should work straight away, right? We don't expect anything to be broken or do we?

Also, there is another option. We could play with the build system, implement some C-style macros like ifdef and then run the app or integration tests with either "javax" or "jakarta" stuff based on some environment flag or something. Of course, that would be a lot of work and perhaps is very much not worth it. Just wanted to mention it for completeness.

@f4lco
Copy link
Collaborator

f4lco commented Apr 27, 2020

make a branch for the "jakarta" things and leave master to use the "javax" stuff for now because Tomcat 10 is still in alpha

Yes, that's reasonable, too. The main takeaway from the branching model is that I assume there are two branches to deploy from for an extended period of time (the transition period between javax and jakarta; former for bugfixes and the latter for ongoing development).

I'm not sure I understand why point 3 is in your list - isn't the "work" there just renaming imports?

Yes, you are totally right, technically it is just doing the imports. However, this is still the area of most uncertainty, because we do not know exactly which other breaking changes Jetty 10 or Tomcat 10 may introduce. Ideally, all tests simply and reliably pass.

Thanks @boris-petrov for your quick response, hearing feedback and agreeing on what to do in the scope of this issue is a good first step in direction of Tomcat 10.

@boris-petrov
Copy link
Member

boris-petrov commented Apr 28, 2020

@f4lco - no, thank you for the time you spend on this. I gave you commit rights to the repository - you can go ahead with creating the branch and porting over your changes from your branch to it when you have the time. 😄

@f4lco
Copy link
Collaborator

f4lco commented Apr 28, 2020

Cool @boris-petrov I did not see that one coming 😃 This gave me extra motivation to spend two spare hours in which I discovered that the tests indeed do not magically pass. At least there are deprecations in the SSL config of Tomcat 10 which require rework.

@boris-petrov
Copy link
Member

@f4lco - I see. It's great to see progress! I doubt that the changes needed will be big - I remember that we did some refactoring to support Tomcat 8.5 and 9 (in order to DRY up a bunch of stuff) but after that everything worked fine. I hope it won't be too difficult now either.

Please keep us posted on the changes and problems - I'll try to help as much as I can. :)

f4lco added a commit that referenced this issue May 6, 2020
The WebappLoader lost the ctor with the parent class
loader parameter and instead sets the reference from
the started context.
Alas, it should be possible to directly set the parent
class loader on the context, and it should just function
as before.
f4lco added a commit that referenced this issue May 6, 2020
The SSL configuration of Tomcat 9 does not work any longer.
The existing implementation used deprecated property names.
Additionally, the SSLHostConfig configuration object is no
longer created automatically if missing.
Compare:
http://tomcat.apache.org/tomcat-9.0-doc/config/http.html#SSL_Support_-_Connector_-_NIO_and_NIO2_(deprecated)
to:
http://tomcat.apache.org/tomcat-10.0-doc/config/http.html#SSL_Support_-_SSLHostConfig
f4lco added a commit that referenced this issue May 6, 2020
Calling toString() is mandatory, otherwise the call does not
match the method signature of setProperty(String, String).
This is now analog to the treatment of httpIdleTimeout, which
already stringifies the property value.
f4lco added a commit that referenced this issue May 6, 2020
Connector#setProperty is a bit of a lie. It does not set properties
on the Connector. Rather, the method set properties on the protocol
handler which is part of the connector.
Thus, the previous implementation tried to set a connector property
using setProperty, which was never successful.
Instead we directly set the property of the connector. To achieve
the original intent ("unlimited post size") the value must be
below zero. Compare:
http://tomcat.apache.org/tomcat-10.0-doc/config/http.html#Common_Attributes
f4lco added a commit that referenced this issue May 6, 2020
Connector#setProperty returns a success boolean. If the property
is deprecated or goes missing, we will immediately notice.
f4lco added a commit that referenced this issue May 6, 2020
While a recent Jersey version provides "jakarta"-ified APIs,
we are still unable to deploy it because Jersey depends on
the javax.servlet classes internally (for instance in
org.glassfish.jersey.servlet.ServletContainer).
We'll have to wait for a compatible release of Jersey.
f4lco added a commit that referenced this issue May 6, 2020
There is already some preliminary work which pulls in
dependencies for Jetty 10. Because Jetty 10 raises the
minimum supported Java version to 11, Gretty will
also require JDK 11 to compile and run.
Source: https://www.eclipse.org/jetty/documentation/current/what-jetty-version.html
f4lco added a commit that referenced this issue May 6, 2020
@f4lco f4lco self-assigned this May 6, 2020
f4lco added a commit that referenced this issue May 8, 2020
The SSL configuration of Tomcat 9 does not work any longer.
The existing implementation used deprecated property names.
Additionally, the SSLHostConfig configuration object is no
longer created automatically if missing.
Compare:
http://tomcat.apache.org/tomcat-9.0-doc/config/http.html#SSL_Support_-_Connector_-_NIO_and_NIO2_(deprecated)
to:
http://tomcat.apache.org/tomcat-10.0-doc/config/http.html#SSL_Support_-_SSLHostConfig
f4lco added a commit that referenced this issue May 8, 2020
Calling toString() is mandatory, otherwise the call does not
match the method signature of setProperty(String, String).
This is now analog to the treatment of httpIdleTimeout, which
already stringifies the property value.
f4lco added a commit that referenced this issue May 8, 2020
Connector#setProperty is a bit of a lie. It does not set properties
on the Connector. Rather, the method set properties on the protocol
handler which is part of the connector.
Thus, the previous implementation tried to set a connector property
using setProperty, which was never successful.
Instead we directly set the property of the connector. To achieve
the original intent ("unlimited post size") the value must be
below zero. See:
http://tomcat.apache.org/tomcat-10.0-doc/config/http.html#Common_Attributes
f4lco added a commit that referenced this issue May 8, 2020
Connector#setProperty returns a success boolean. If the property
is deprecated or goes missing, we will immediately notice.
f4lco added a commit that referenced this issue May 18, 2020
The WebappLoader lost the ctor with the parent class
loader parameter and instead sets the reference from
the started context.
Alas, it should be possible to directly set the parent
class loader on the context, and it should just function
as before.
f4lco added a commit that referenced this issue May 18, 2020
While a recent Jersey version provides "jakarta"-ified APIs,
we are still unable to deploy it because Jersey depends on
the javax.servlet classes internally (for instance in
org.glassfish.jersey.servlet.ServletContainer).
We'll have to wait for a compatible release of Jersey.
f4lco added a commit that referenced this issue May 18, 2020
There is already some preliminary work which pulls in
dependencies for Jetty 10. Because Jetty 10 raises the
minimum supported Java version to 11, Gretty will
also require JDK 11 to compile and run.
Source: https://www.eclipse.org/jetty/documentation/current/what-jetty-version.html
@f4lco
Copy link
Collaborator

f4lco commented May 22, 2020

Closing: the Tomcat 10 connector landed in long-lived feature branch "gretty4". I assume it will take time to release anything Jakarta-related. A milestone is in place: https://github.com/gretty-gradle-plugin/gretty/milestone/2

@f4lco f4lco closed this as completed May 22, 2020
@f4lco f4lco linked a pull request May 22, 2020 that will close this issue
f4lco added a commit that referenced this issue Sep 15, 2020
The WebappLoader lost the ctor with the parent class
loader parameter and instead sets the reference from
the started context.
Alas, it should be possible to directly set the parent
class loader on the context, and it should just function
as before.
f4lco added a commit that referenced this issue Sep 15, 2020
While a recent Jersey version provides "jakarta"-ified APIs,
we are still unable to deploy it because Jersey depends on
the javax.servlet classes internally (for instance in
org.glassfish.jersey.servlet.ServletContainer).
We'll have to wait for a compatible release of Jersey.
f4lco added a commit that referenced this issue Sep 15, 2020
There is already some preliminary work which pulls in
dependencies for Jetty 10. Because Jetty 10 raises the
minimum supported Java version to 11, Gretty will
also require JDK 11 to compile and run.
Source: https://www.eclipse.org/jetty/documentation/current/what-jetty-version.html
f4lco added a commit that referenced this issue Sep 17, 2020
The WebappLoader lost the ctor with the parent class
loader parameter and instead sets the reference from
the started context.
Alas, it should be possible to directly set the parent
class loader on the context, and it should just function
as before.
f4lco added a commit that referenced this issue Sep 17, 2020
While a recent Jersey version provides "jakarta"-ified APIs,
we are still unable to deploy it because Jersey depends on
the javax.servlet classes internally (for instance in
org.glassfish.jersey.servlet.ServletContainer).
We'll have to wait for a compatible release of Jersey.
f4lco added a commit that referenced this issue Sep 17, 2020
There is already some preliminary work which pulls in
dependencies for Jetty 10. Because Jetty 10 raises the
minimum supported Java version to 11, Gretty will
also require JDK 11 to compile and run.
Source: https://www.eclipse.org/jetty/documentation/current/what-jetty-version.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants