Skip to content

Commit

Permalink
Issue #5022 Filter Cache cleanup (#5271)
Browse files Browse the repository at this point in the history 
* Issue #5022 Filter Cache cleanup

Issue #5022 Filter Cache cleanup:
 + Fixed many compiler warnings
 + removed old LazyList leftovers
 + Don't create holder string for source unless required
 + Only have a single type of chain, so it can be wrapped regardless of cache
 + Reverse mappings lists to make filter chain creation easier
 + build chain directly rather than build a list then a chain

Signed-off-by: Greg Wilkins <gregw@webtide.com>

* added comment to explain ordering

Signed-off-by: gregw <gregw@webtide.com>

* More cleanups

* fixed toString format
turn off debug in OSGI test
  • Loading branch information
@gregw
gregw authored Oct 5, 2020
1 parent 7fcae31 commit c40b955
Show file tree
Hide file tree
Showing 4 changed files with 143 additions and 279 deletions.
4 changes: 2 additions & 2 deletions ...sgi/test-jetty-osgi/src/test/java/org/eclipse/jetty/osgi/test/TestJettyOSGiBootHTTP2.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public Option[] config()
options.add(mavenBundle().groupId("org.eclipse.jetty.http2").artifactId("http2-http-client-transport").versionAsInProject().start());

options.add(systemProperty("org.ops4j.pax.logging.DefaultServiceLog.level").value(LOG_LEVEL));
options.add(systemProperty("org.eclipse.jetty.LEVEL").value("DEBUG"));
options.add(systemProperty("org.eclipse.jetty.LEVEL").value("INFO"));
options.add(CoreOptions.cleanCaches(true));
return options.toArray(new Option[0]);
}
Expand Down Expand Up @@ -154,7 +154,7 @@ public void testHTTP2() throws Exception
httpClient.start();

ContentResponse response = httpClient.GET("https://localhost:" + port + "/jsp/jstl.jsp");
assertEquals(response.toString(), response.getStatus(), HttpStatus.OK_200);
assertEquals(HttpStatus.OK_200,response.getStatus());
String body = response.getContentAsString();
assertTrue("Body contains \"JSTL Example\": " + body, body.contains("JSTL Example"));
}
Expand Down
4 changes: 2 additions & 2 deletions jetty-server/src/main/java/org/eclipse/jetty/server/Request.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -195,7 +195,7 @@ public static Request getBaseRequest(ServletRequest request)
private String _servletPath;
private String _pathInfo;
private boolean _secure;
private String _asyncNotSupportedSource = null;
private Object _asyncNotSupportedSource = null;
private boolean _newContext;
private boolean _cookiesExtracted = false;
private boolean _handled = false;
Expand Down Expand Up @@ -1952,7 +1952,7 @@ public void removeEventListener(final EventListener listener)
_requestAttributeListeners.remove(listener);
}

public void setAsyncSupported(boolean supported, String source)
public void setAsyncSupported(boolean supported, Object source)

This comment has been minimized.

Sign in to view

Copy link
@bentmann

bentmann Oct 23, 2020

This signature change is not binary compatible. Was this intended to go out with 9.4.33?

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Oct 24, 2020

Author Contributor

It was intended to be included in 9.4.33 as we don't consider this as a public API. Yes I know it is a public method on a public class, but then so are most of the methods in our implementation.

The change to Object from String is a substantial saving on garbage as it prevents a toString being called on a potentially complex object on every filter pass.

But given that you are using it, then I guess our estimate of what users might use is not correct. What is your usage situation ? are you calling it, extending it or wrapping it? We could put a deprecated string version in the next version, but that will not help much if you are extending it and expect Jetty to call the string version.

This comment has been minimized.

Sign in to view

Copy link
@bentmann

bentmann Oct 24, 2020

What is your usage situation ?

A mere call:
https://github.com/dropwizard/dropwizard/blob/release/1.3.x/dropwizard-jetty/src/main/java/io/dropwizard/jetty/NonblockingServletHolder.java#L46

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Oct 25, 2020

Author Contributor

@bentmann That class looks like an attempt to avoid some lock contention. It really REALLY isn't the kind of thing that should be attempted by extension as it is eliding a lot of behaviour that may change from release to release.

As a project, we are really open to feedback, including on flexibility and performance. So if Dropwizard was seeing contention on that lock, then the best thing to do is raise the issue with us so we can see if we can do something in the core project.

I've had a look at the history of that class and it goes back 9 years to when it was moved, so I can't easily see why it was created. But Jetty from 9 years ago is significantly different to jetty now, specially in how servlets are initialized (As the servlet spec has evolved and been clarified).

I expect that any optimisation this class is based on is likely to need review. Can you find the original issue that caused this class to be added?

Meanwhile, I'd advise simply removing that class and using normal jetty holders. IF you have contention or performance problems then tell us and we'll do something. We can't hold our internals static for 3rd party optimisations done more than 9 years ago that we don't know about.

cheers

This comment has been minimized.

Sign in to view

Copy link
@gregw

gregw Oct 25, 2020

Author Contributor

See #5498
{
_asyncNotSupportedSource = supported ? null : (source == null ? "unknown" : source);
}
Expand Down
22 changes: 18 additions & 4 deletions jetty-servlet/src/main/java/org/eclipse/jetty/servlet/FilterHolder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;

import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.util.TypeUtil;
import org.eclipse.jetty.util.component.Dumpable;
import org.eclipse.jetty.util.component.DumpableCollection;
Expand Down Expand Up @@ -186,11 +187,24 @@ public Filter getFilter()
return _filter;
}

public void doFilter(ServletRequest request, ServletResponse response,
FilterChain chain)
throws IOException, ServletException
public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException
{
_filter.doFilter(request, response, chain);
if (isAsyncSupported() || !request.isAsyncSupported())
getFilter().doFilter(request, response, chain);
else
{
Request baseRequest = Request.getBaseRequest(request);
Objects.requireNonNull(baseRequest);
try
{
baseRequest.setAsyncSupported(false, this);
getFilter().doFilter(request, response, chain);
}
finally
{
baseRequest.setAsyncSupported(true, null);
}
}
}

@Override
Expand Down
Loading

0 comments on commit c40b955

Please sign in to comment.