Improve start up performance by initializing servlets only once #53

wb459 · 2015-09-30T11:40:56Z

In Jetty 9+ the ServletHandler.initialize method will do calls to ServletHolder.start and ServletHolder.initialize. In Jetty 8, initialization was part of the ServletHolder.start method. Since the initialization was part of the start method, its execution was also bound on the same conditions (being: the servlet is not started or starting). In Jetty 9, ServletHolder.initialize is called regardless of the state of the servlet. This new behavior is causing a great increase in start-up times of some applications.

To tackle this issue, a new 'initialized' property is introduced on the ServletHolder which will be set to true after the first call to ServletHolder.initialize to prevent re-initializing initialized servlets.

It's not possible to fix this issue by making the ServletHolder.initialize part of the ServletHolder.start again because this will result in IllegalStateExceptions because the servlet has to be started (AbstractLifeCycle.setStarted) before ServletHolder.initialize can be called.

joakime · 2015-09-30T12:59:44Z

A quick look at the implementation on the Jetty side ...

Are you using ServletHandler directly?
They are meant to be used from within a ServletContextHandler, not directly on the Handler tree with Jetty.

wb459 · 2015-09-30T13:08:36Z

We're using Jetty via the pax-web-jetty-bundle (http://mvnrepository.com/artifact/org.ops4j.pax.web/pax-web-jetty-bundle). The specific call that causes the slow start-up is the ServletHandler.addFilter call that is done (directly) from org.ops4j.pax.web.service.jetty.internal.JettyServerImpl.addFilter. We have no control over this Pax Web implementation.

Is something inherently wrong about my commit? Including the commit would resolve our issues as soon as Pax Web would include the newly released jetty-servlet artifact.

joakime · 2015-09-30T14:53:47Z

Sounds like the problem is on the pax-web side.
We are not against this suggestion, but just want to know more about the situation and causes, as the change will have an impact on nearly 98% of our users (the overwhelming majority of which do not use OSGi).

Also, before we can accept this, you have to have a signed CLA with the Eclipse Foundation.
Follow the "ip-validation" "Details" link in this PR for details.

gregw · 2015-09-30T22:01:20Z

@WouterBanckenACA I'm trying to work out how initialize can be called multiple times.

It is primarily called from either ServletHandler doStart (if there is no context handler):

        if (_contextHandler==null)
            initialize();

or from the servletContextHandler do start (if it exists):

        if (_servletHandler != null)
            _servletHandler.initialize();

The only other call to initialize is from updateMappings, which does test state:

            if (_contextHandler!=null && _contextHandler.isStarted() || _contextHandler==null && isStarted())
                initialize();

updateMappings is called from several places.... so I guess if you are calling it multiple times on a started context, then we may be multi initializing the servlet/filters!

So I do think this needs to be fixed. If you want credit for the change, please do the ip-validation step and we can look at doing the merge. Otherwise I'll separately invent a solution.

janbartel · 2015-09-30T23:07:54Z

@WouterBanckenACA just for the record, could you do new Throwable().printStackTrace() from within your servlet init method and paste it so we can see the trace of the calls that produces this situation? It must be as Greg says, and that paxweb is adding another servlet/filter after the servlet context has already started via the jetty-specific api, which is not something that is possible with the standard servlet spec api.

wb459 · 2015-10-01T08:40:08Z

@janbartel The issues are caused when the initialization is done from the following stacktrace:

java.lang.Throwable
at org.eclipse.jetty.servlet.ServletHolder.initialize(ServletHolder.java:390)
at org.eclipse.jetty.servlet.ServletHandler.initialize(ServletHandler.java:871)
at org.eclipse.jetty.servlet.ServletHandler.updateMappings(ServletHandler.java:1526)
at org.eclipse.jetty.servlet.ServletHandler.setFilterMappings(ServletHandler.java:1560)
at org.eclipse.jetty.servlet.ServletHandler.addFilterMapping(ServletHandler.java:1233)
at org.eclipse.jetty.servlet.ServletHandler.addFilter(ServletHandler.java:1208)
at org.ops4j.pax.web.service.jetty.internal.JettyServerImpl$4.call(JettyServerImpl.java:528)
at org.ops4j.pax.web.service.jetty.internal.JettyServerImpl$4.call(JettyServerImpl.java:524)
at org.ops4j.pax.swissbox.core.ContextClassLoaderUtils.doWithClassLoader(ContextClassLoaderUtils.java:60)
at org.ops4j.pax.web.service.jetty.internal.JettyServerImpl.addFilter(JettyServerImpl.java:523)
at org.ops4j.pax.web.service.jetty.internal.ServerControllerImpl$Started.addFilter(ServerControllerImpl.java:315)
at org.ops4j.pax.web.service.jetty.internal.ServerControllerImpl.addFilter(ServerControllerImpl.java:145)
at org.ops4j.pax.web.service.internal.HttpServiceStarted.registerFilter(HttpServiceStarted.java:484)
at org.ops4j.pax.web.service.internal.HttpServiceProxy.registerFilter(HttpServiceProxy.java:188)
(proprietary code)

@gregw I've done the ip-validation. Do I have to recommit to pass the checks?
Other than that the credit is not too important to me so if its easier if you do the fix, then that's fine for me as well.

tomdw · 2015-10-02T18:06:53Z

Any idea when this helpful fix would be merged and included in a intermediate release so that the community can benefit from it? If the ip-validation is to slow, please do as Wouter suggests to re-apply the same changes in another pull request by someone who can merge already.

wb459 · 2015-10-07T08:43:47Z

@gregw @janbartel @joakime Any updates on this issue? Will the fix be included?

joakime · 2015-10-07T16:57:44Z

Just confirming that @WouterBanckenACA now shows up as having a valid CLA on file using the validation tool at https://projects.eclipse.org/user/cla/validate

janbartel · 2015-10-08T00:01:56Z

Wouter, I've applied this change to the jetty-9.2.x branch and am in the middle of applying it to master. Worth noting: I had to make 2 changes:

class members that are not static are prefixed with "_" character
need to reset _initialized to false in doStop() so that context restarts work correctly

These changes didn't make it in in time for 9.3.4 release, so they'll be in 9.3.5.

regards
Jan

tomdw · 2015-10-10T08:32:08Z

@janbartel is there any timeframe known when the 9.3.5 release will be created so that this huge performance gain can be integrated in our systems?

Of if it is possible to create an intermediate 9.2.x release including the fix quicker that would also help.

wb459 · 2015-10-26T09:08:55Z

@janbartel @joakime @gregw Thanks for including the fix! Do you have any idea when you will do a 9.2.14 release? We're having issues upgrading our projects to Java 8 but we'd still like to include this fix.

wardvijf · 2015-10-29T08:39:24Z

@janbartel @joakime @gregw Any updates on this one? We would really like to have this in a 9.2.14 release so we can make our system startup time acceptable again.

joakime · 2015-10-29T16:34:09Z

9.3.5.v20151012 is released, with this fix.

There are no 9.2.x releases currently on the schedule (but that does not mean there will never be)

wb459 · 2015-10-29T18:05:34Z

@joakime We tried to use the 9.3.5.v20151012 artifact but all 9.3.x releases require java 8. There are some other third party frameworks that are currently limiting us to use java 7 so it would be very helpful if there would be a jetty 9.2.x release since the 9.2.x code base also includes this fix. We are currently on a migration path to java 8 but we'd like to include this jetty fix in our code base asap since the bug is causing a very slow startup of our applications.

tomdw · 2015-10-30T15:35:23Z

@joakime @janbartel you indicated that a 9.2.x release including the fix might be possible. How can we get such a small release planned in so that non-jdk8 projects (which cannot upgrade to 9.3) can upgrade to a version of jetty which has fast servlet registration?

It would be great to have an idea quickly of when this is possible so we know which options we have.

Thanks in advance.

joakime · 2015-11-02T12:55:48Z

I'll bring this up on the weekly release discussions (on Wednesdays).

We have 3 active branches at the moment.

Servlet 4.0 / Java 9 / Jetty 10
Jetty 9.4
Jetty 9.3

Jetty 9.2 is after those, from a time management and priority point of view.
Since Java 7 itself is no longer supported by Oracle, we have taken the same viewpoint about it.

Our time is also heavily used for our extended support clients (Backport of bug fixes to as far back as Jetty 8.0.x and migration support to Jetty 9.3 from as far back as Jetty 6.0)

jmcc0nn3ll · 2015-11-03T15:10:51Z

There will be another 9.2.x release in the not hugely distant future, there are a couple of other issues that will be looped into it as well. Webtide has largely committed to keeping 9.2.x updated in maintenance mode to support java 7 users but from an open source perspective it is as @joakime indicated fairly low on the totem pole. As clients need releases we will update and release as well of course (same with jetty 7 and jetty8). Remember that from an SSL perspective no one should be using Java7 any longer...which is what has prompted most companies we know to have made the switch to Java8 already.

joakime · 2015-11-06T20:28:05Z

The staged/potential release of Jetty 9.2.14 is available at the oss.sonatype.org maven/nexus staging repository url:

https://oss.sonatype.org/content/repositories/jetty-1201

Version:

9.2.14.v20151106

Tag:

https://github.com/eclipse/jetty.project/tree/jetty-9.2.14.v20151106

Commit Log:

https://github.com/eclipse/jetty.project/commits/jetty-9.2.14.v20151106

Direct Link to jetty-distribution:

https://oss.sonatype.org/content/repositories/jetty-1201/org/eclipse/jetty/jetty-distribution/9.2.14.v20151106/jetty-distribution-9.2.14.v20151106.tar.gz

As usual, we'll let this staged copy sit for a few days to be tested, if there are no reported issues, we will release it.

Important Note: no versions of Jetty are considered official until they are approved for release.

tomdw · 2015-11-12T11:48:18Z

@joakime what is defined a 'a few days'? 1 week? 1 month?

joakime · 2015-11-12T13:26:56Z

it went live on maven central last night.

http://search.maven.org/#search%7Cga%7C1%7Cg%3A%22org.eclipse.jetty%22%20AND%20v%3A%229.2.14.v20151106%22

Still working on other periphery parts of the release (that's why there's not yet been an announcement)
Things like the p2 repository, main eclipse download sites, rolling older releases to archives, documentation, etc...

Add initialized boolean to ServletHolder

144b49e

janbartel self-assigned this Oct 7, 2015

janbartel closed this Oct 8, 2015

yogeshbelur mentioned this pull request Jan 18, 2018

Server is hung due to cache refresh #2141

Closed

SarthakSahu mentioned this pull request Mar 27, 2020

REST endpoint stops responding after sometime. #4724

Closed

ops4j-issues mentioned this pull request Feb 25, 2021

Investigate if the current calls to Jetty's ServletHandler are done correctly [PAXWEB-891] ops4j/org.ops4j.pax.web#1175

Closed

Spandana806 mentioned this pull request Nov 3, 2023

Multiple WebSocket threads getting created for every single client connection #10850

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve start up performance by initializing servlets only once #53

Improve start up performance by initializing servlets only once #53

wb459 commented Sep 30, 2015

joakime commented Sep 30, 2015

wb459 commented Sep 30, 2015

joakime commented Sep 30, 2015

gregw commented Sep 30, 2015

janbartel commented Sep 30, 2015

wb459 commented Oct 1, 2015

tomdw commented Oct 2, 2015

wb459 commented Oct 7, 2015

joakime commented Oct 7, 2015

janbartel commented Oct 8, 2015

tomdw commented Oct 10, 2015

wb459 commented Oct 26, 2015

wardvijf commented Oct 29, 2015

joakime commented Oct 29, 2015

wb459 commented Oct 29, 2015

tomdw commented Oct 30, 2015

joakime commented Nov 2, 2015

jmcc0nn3ll commented Nov 3, 2015

joakime commented Nov 6, 2015

tomdw commented Nov 12, 2015

joakime commented Nov 12, 2015

Improve start up performance by initializing servlets only once #53

Improve start up performance by initializing servlets only once #53

Conversation

wb459 commented Sep 30, 2015

joakime commented Sep 30, 2015

wb459 commented Sep 30, 2015

joakime commented Sep 30, 2015

gregw commented Sep 30, 2015

janbartel commented Sep 30, 2015

wb459 commented Oct 1, 2015

tomdw commented Oct 2, 2015

wb459 commented Oct 7, 2015

joakime commented Oct 7, 2015

janbartel commented Oct 8, 2015

tomdw commented Oct 10, 2015

wb459 commented Oct 26, 2015

wardvijf commented Oct 29, 2015

joakime commented Oct 29, 2015

wb459 commented Oct 29, 2015

tomdw commented Oct 30, 2015

joakime commented Nov 2, 2015

jmcc0nn3ll commented Nov 3, 2015

joakime commented Nov 6, 2015

tomdw commented Nov 12, 2015

joakime commented Nov 12, 2015