From 182d6d4708dbb197d09e570c9f3ca782644eb3a3 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 17 Nov 2022 16:20:16 +1100 Subject: [PATCH 01/12] recycle ServletChannel WIP --- .../ee10/servlet/AsyncContentProducer.java | 11 +- .../jetty/ee10/servlet/AsyncContextState.java | 2 +- .../ee10/servlet/BlockingContentProducer.java | 6 +- .../eclipse/jetty/ee10/servlet/HttpInput.java | 52 +++--- .../jetty/ee10/servlet/ServletChannel.java | 148 +++++++++--------- .../ee10/servlet/ServletContextHandler.java | 9 +- .../ee10/servlet/ServletContextRequest.java | 4 +- .../ee10/servlet/ServletContextResponse.java | 2 +- .../ee10/servlet/ServletRequestState.java | 18 +-- 9 files changed, 128 insertions(+), 124 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index f154425dfd22..3a351ddfc772 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java @@ -38,7 +38,7 @@ class AsyncContentProducer implements ContentProducer private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); private static final Content.Chunk.Error RECYCLED_ERROR_CHUNK = Content.Chunk.from(new StaticException("ContentProducer has been recycled")); - private final AutoLock _lock = new AutoLock(); + private final AutoLock _lock; private final ServletChannel _servletChannel; private HttpInput.Interceptor _interceptor; private Content.Chunk _rawChunk; @@ -47,9 +47,10 @@ class AsyncContentProducer implements ContentProducer private long _firstByteNanoTime = Long.MIN_VALUE; private long _rawBytesArrived; - AsyncContentProducer(ServletChannel servletChannel) + AsyncContentProducer(ServletChannel servletChannel, AutoLock lock) { _servletChannel = servletChannel; + _lock = lock; } @Override @@ -225,7 +226,7 @@ private boolean consumeCurrentChunk() private boolean consumeAvailableChunks() { - ServletContextRequest request = _servletChannel.getRequest(); + ServletContextRequest request = _servletChannel.getServletContextRequest(); while (true) { Content.Chunk chunk = request.read(); @@ -288,7 +289,7 @@ public boolean isReady() } _servletChannel.getState().onReadUnready(); - _servletChannel.getRequest().demand(() -> + _servletChannel.getServletContextRequest().demand(() -> { if (_servletChannel.getHttpInput().onContentProducible()) _servletChannel.handle(); @@ -481,7 +482,7 @@ else if (chunk != _rawChunk && !_rawChunk.isTerminal() && _rawChunk.hasRemaining private Content.Chunk produceRawChunk() { - Content.Chunk chunk = _servletChannel.getRequest().read(); + Content.Chunk chunk = _servletChannel.getServletContextRequest().read(); if (chunk != null) { _rawBytesArrived += chunk.remaining(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContextState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContextState.java index abf396ddfa6d..a05c623e0a3b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContextState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContextState.java @@ -116,7 +116,7 @@ public boolean hasOriginalRequestAndResponse() ServletRequest servletRequest = getRequest(); ServletResponse servletResponse = getResponse(); ServletChannel servletChannel = _state.getServletChannel(); - HttpServletRequest originalHttpServletRequest = servletChannel.getRequest().getHttpServletRequest(); + HttpServletRequest originalHttpServletRequest = servletChannel.getServletContextRequest().getHttpServletRequest(); HttpServletResponse originalHttpServletResponse = servletChannel.getResponse().getHttpServletResponse(); return (servletRequest == originalHttpServletRequest && servletResponse == originalHttpServletResponse); } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java index 0f51f856953c..9c7238b96b9b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java @@ -28,17 +28,19 @@ class BlockingContentProducer implements ContentProducer private final AsyncContentProducer _asyncContentProducer; private final AsyncContentProducer.LockedSemaphore _semaphore; + private final AutoLock _lock; - BlockingContentProducer(AsyncContentProducer delegate) + BlockingContentProducer(AsyncContentProducer delegate, AutoLock lock) { _asyncContentProducer = delegate; + _lock = lock; _semaphore = _asyncContentProducer.newLockedSemaphore(); } @Override public AutoLock lock() { - return _asyncContentProducer.lock(); + return _lock.lock(); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index ca6149852cb6..68cec4e7edb9 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -37,11 +37,12 @@ public class HttpInput extends ServletInputStream implements Runnable { private static final Logger LOG = LoggerFactory.getLogger(HttpInput.class); + private final AutoLock _lock = new AutoLock(); private final ServletChannel _servletChannel; + private final ServletRequestState _channelState; private final byte[] _oneByteBuffer = new byte[1]; - private BlockingContentProducer _blockingContentProducer; - private AsyncContentProducer _asyncContentProducer; - private ServletRequestState _channelState; + private final BlockingContentProducer _blockingContentProducer; + private final AsyncContentProducer _asyncContentProducer; private final LongAdder _contentConsumed = new LongAdder(); private volatile ContentProducer _contentProducer; private volatile boolean _consumedEof; @@ -50,22 +51,27 @@ public class HttpInput extends ServletInputStream implements Runnable public HttpInput(ServletChannel channel) { _servletChannel = channel; + _channelState = _servletChannel.getState(); + _asyncContentProducer = new AsyncContentProducer(_servletChannel, _lock); + _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer, _lock); + _contentProducer = _blockingContentProducer; } - // TODO avoid this init() public void recycle() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { if (LOG.isDebugEnabled()) LOG.debug("recycle {}", this); _blockingContentProducer.recycle(); + _contentProducer = _blockingContentProducer; } } public void reopen() { - try (AutoLock lock = _contentProducer.lock()) + // TODO why is this never used? + try (AutoLock lock = _lock.lock()) { if (LOG.isDebugEnabled()) LOG.debug("reopen {}", this); @@ -77,20 +83,12 @@ public void reopen() } } - public void init() - { - _channelState = _servletChannel.getState(); // TODO can we change lifecycle so this is known in constructor and can be final - _asyncContentProducer = new AsyncContentProducer(_servletChannel); // TODO avoid object creation or recycle - _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer); // TODO avoid object creation or recycle - _contentProducer = _blockingContentProducer; - } - /** * @return The current Interceptor, or null if none set */ public Interceptor getInterceptor() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { return _contentProducer.getInterceptor(); } @@ -103,7 +101,7 @@ public Interceptor getInterceptor() */ public void setInterceptor(Interceptor interceptor) { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { if (LOG.isDebugEnabled()) LOG.debug("setting interceptor to {} on {}", interceptor, this); @@ -119,7 +117,7 @@ public void setInterceptor(Interceptor interceptor) */ public void addInterceptor(Interceptor interceptor) { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { Interceptor currentInterceptor = _contentProducer.getInterceptor(); if (currentInterceptor == null) @@ -173,7 +171,7 @@ public long getContentConsumed() public long getContentReceived() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { return _contentProducer.getRawBytesArrived(); } @@ -181,7 +179,7 @@ public long getContentReceived() public boolean consumeAvailable() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { if (LOG.isDebugEnabled()) LOG.debug("consumeAll {}", this); @@ -198,7 +196,7 @@ public boolean consumeAvailable() public boolean isError() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { boolean error = _contentProducer.isError(); if (LOG.isDebugEnabled()) @@ -228,7 +226,7 @@ public boolean isFinished() @Override public boolean isReady() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { boolean ready = _contentProducer.isReady(); if (LOG.isDebugEnabled()) @@ -257,7 +255,7 @@ public void setReadListener(ReadListener readListener) public boolean onContentProducible() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { return _contentProducer.onContentProducible(); } @@ -266,7 +264,7 @@ public boolean onContentProducible() @Override public int read() throws IOException { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { int read = read(_oneByteBuffer, 0, 1); if (read == 0) @@ -288,7 +286,7 @@ public int read(ByteBuffer buffer) throws IOException private int read(ByteBuffer buffer, byte[] b, int off, int len) throws IOException { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { // Don't try to get content if no bytes were requested to be read. if (len == 0) @@ -342,7 +340,7 @@ private void scheduleReadListenerNotification() */ public boolean hasContent() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { // Do not call _contentProducer.available() as it calls HttpChannel.produceContent() // which is forbidden by this method's contract. @@ -356,7 +354,7 @@ public boolean hasContent() @Override public int available() { - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { int available = _contentProducer.available(); if (LOG.isDebugEnabled()) @@ -375,7 +373,7 @@ public int available() public void run() { Content.Chunk chunk; - try (AutoLock lock = _contentProducer.lock()) + try (AutoLock lock = _lock.lock()) { // Call isReady() to make sure that if not ready we register for fill interest. if (!_contentProducer.isReady()) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 786e1c6b5b7c..b58637f9c5bc 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -64,21 +64,33 @@ public class ServletChannel implements Runnable { private static final Logger LOG = LoggerFactory.getLogger(ServletChannel.class); + private final ServletRequestState _state; private final AtomicLong _requests = new AtomicLong(); - private Connector _connector; - private Executor _executor; - private HttpConfiguration _configuration; - private EndPoint _endPoint; - private ServletRequestState _state; + private final Connector _connector; + private final Executor _executor; + private final HttpConfiguration _configuration; + private final EndPoint _endPoint; + private final HttpInput _httpInput; + private final Listener _combinedListener; private ServletContextHandler.ServletContextApi _servletContextApi; - private ServletContextRequest _request; + private ServletContextRequest _servletContextRequest; private boolean _expects100Continue; - private Listener _combinedListener; private long _oldIdleTimeout; private Callback _callback; // Bytes written after interception (e.g. after compression). private long _written; + public ServletChannel(ServletContextHandler servletContextHandler, Request request) + { + _state = new ServletRequestState(this); + _connector = request.getConnectionMetaData().getConnector(); + _executor = request.getContext(); + _configuration = request.getConnectionMetaData().getHttpConfiguration(); + _endPoint = request.getConnectionMetaData().getConnection().getEndPoint(); + _httpInput = new HttpInput(this); + _combinedListener = new Listeners(_connector, servletContextHandler); + } + public void setCallback(Callback callback) { if (_callback != null) @@ -86,30 +98,24 @@ public void setCallback(Callback callback) _callback = callback; } - public void init(ServletContextRequest request) + public void init(ServletContextRequest servletContextRequest) { - _connector = request.getConnectionMetaData().getConnector(); - _executor = request.getContext(); - _configuration = request.getConnectionMetaData().getHttpConfiguration(); - _endPoint = request.getConnectionMetaData().getConnection().getEndPoint(); - _state = new ServletRequestState(this); // TODO can this be recycled? - _servletContextApi = request.getContext().getServletContext(); - _request = request; - _expects100Continue = request.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()); - _combinedListener = new Listeners(request); - - request.getHttpInput().init(); + _state.recycle(); + _httpInput.recycle(); + _servletContextApi = servletContextRequest.getContext().getServletContext(); + _servletContextRequest = servletContextRequest; + _expects100Continue = servletContextRequest.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()); if (LOG.isDebugEnabled()) LOG.debug("new {} -> {},{}", this, - _request, + _servletContextRequest, _state); } public ServletContextHandler.Context getContext() { - return _request.getContext(); + return _servletContextRequest.getContext(); } public ServletContextHandler getContextHandler() @@ -124,12 +130,12 @@ public ServletContextHandler.ServletContextApi getServletContext() public HttpOutput getHttpOutput() { - return _request.getHttpOutput(); + return _servletContextRequest.getHttpOutput(); } public HttpInput getHttpInput() { - return _request.getHttpInput(); + return _httpInput; } public ServletContextHandler.ServletContextApi getServletContextContext() @@ -195,14 +201,14 @@ public Server getServer() return _connector.getServer(); } - public ServletContextRequest getRequest() + public ServletContextRequest getServletContextRequest() { - return _request; + return _servletContextRequest; } public ServletContextResponse getResponse() { - return _request.getResponse(); + return _servletContextRequest.getResponse(); } public Connection getConnection() @@ -363,7 +369,7 @@ public void run() public boolean handle() { if (LOG.isDebugEnabled()) - LOG.debug("handle {} {} ", _request.getHttpURI(), this); + LOG.debug("handle {} {} ", _servletContextRequest.getHttpURI(), this); Action action = _state.handling(); @@ -395,9 +401,9 @@ public boolean handle() { ServletContextHandler.ServletContextApi servletContextApi = getServletContextContext(); ServletHandler servletHandler = servletContextApi.getContext().getServletContextHandler().getServletHandler(); - ServletHandler.MappedServlet mappedServlet = _request._mappedServlet; + ServletHandler.MappedServlet mappedServlet = _servletContextRequest._mappedServlet; - mappedServlet.handle(servletHandler, Request.getPathInContext(_request), _request.getHttpServletRequest(), _request.getHttpServletResponse()); + mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getHttpServletRequest(), _servletContextRequest.getHttpServletResponse()); }); break; @@ -408,15 +414,15 @@ public boolean handle() dispatch(DispatcherType.ASYNC, () -> { HttpURI uri; - String pathInContext = Request.getPathInContext(_request); + String pathInContext = Request.getPathInContext(_servletContextRequest); AsyncContextEvent asyncContextEvent = _state.getAsyncContextEvent(); String dispatchString = asyncContextEvent.getDispatchPath(); if (dispatchString != null) { - String contextPath = _request.getContext().getContextPath(); + String contextPath = _servletContextRequest.getContext().getContextPath(); HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); - uri = HttpURI.build(_request.getHttpURI()) + uri = HttpURI.build(_servletContextRequest.getHttpURI()) .path(URIUtil.addPaths(contextPath, pathInContext)) .query(dispatchUri.getQuery()); } @@ -425,13 +431,13 @@ public boolean handle() uri = asyncContextEvent.getBaseURI(); if (uri == null) { - uri = _request.getHttpURI(); + uri = _servletContextRequest.getHttpURI(); } else { pathInContext = uri.getCanonicalPath(); - if (_request.getContext().getContextPath().length() > 1) - pathInContext = pathInContext.substring(_request.getContext().getContextPath().length()); + if (_servletContextRequest.getContext().getContextPath().length() > 1) + pathInContext = pathInContext.substring(_servletContextRequest.getContext().getContextPath().length()); } } @@ -454,7 +460,7 @@ public boolean handle() // the following is needed as you cannot trust the response code and reason // as those could have been modified after calling sendError - Integer code = (Integer)_request.getAttribute(RequestDispatcher.ERROR_STATUS_CODE); + Integer code = (Integer)_servletContextRequest.getAttribute(RequestDispatcher.ERROR_STATUS_CODE); if (code == null) code = HttpStatus.INTERNAL_SERVER_ERROR_500; getResponse().setStatus(code); @@ -464,9 +470,9 @@ public boolean handle() // from the failed dispatch, then we try to consume it here and if we fail we add a // Connection:close. This can't be deferred to COMPLETE as the response will be committed // by then. - Response.ensureConsumeAvailableOrNotPersistent(_request, _request.getResponse()); + Response.ensureConsumeAvailableOrNotPersistent(_servletContextRequest, _servletContextRequest.getResponse()); - ContextHandler.Context context = (ContextHandler.Context)_request.getAttribute(ErrorHandler.ERROR_CONTEXT); + ContextHandler.Context context = (ContextHandler.Context)_servletContextRequest.getAttribute(ErrorHandler.ERROR_CONTEXT); Request.Processor errorProcessor = ErrorHandler.getErrorProcessor(getServer(), context == null ? null : context.getContextHandler()); // If we can't have a body or have no processor, then create a minimal error response. @@ -481,7 +487,7 @@ public boolean handle() // _state.completing(); try (Blocker.Callback blocker = Blocker.callback()) { - dispatch(DispatcherType.ERROR, () -> errorProcessor.process(_request, getResponse(), blocker)); + dispatch(DispatcherType.ERROR, () -> errorProcessor.process(_servletContextRequest, getResponse(), blocker)); blocker.block(); } } @@ -510,7 +516,7 @@ public boolean handle() finally { // clean up the context that was set in Response.sendError - _request.removeAttribute(ErrorHandler.ERROR_CONTEXT); + _servletContextRequest.removeAttribute(ErrorHandler.ERROR_CONTEXT); } break; } @@ -523,14 +529,14 @@ public boolean handle() case READ_CALLBACK: { ServletContextHandler handler = _state.getContextHandler(); - handler.getContext().run(() -> _request.getHttpInput().run()); + handler.getContext().run(() -> _servletContextRequest.getHttpInput().run()); break; } case WRITE_CALLBACK: { ServletContextHandler handler = _state.getContextHandler(); - handler.getContext().run(() -> _request.getHttpOutput().run()); + handler.getContext().run(() -> _servletContextRequest.getHttpOutput().run()); break; } @@ -550,14 +556,14 @@ public boolean handle() // Indicate Connection:close if we can't consume all. if (getResponse().getStatus() >= 200) - Response.ensureConsumeAvailableOrNotPersistent(_request, _request.getResponse()); + Response.ensureConsumeAvailableOrNotPersistent(_servletContextRequest, _servletContextRequest.getResponse()); } // RFC 7230, section 3.3. - if (!_request.isHead() && + if (!_servletContextRequest.isHead() && getResponse().getStatus() != HttpStatus.NOT_MODIFIED_304 && - !getResponse().isContentComplete(_request.getHttpOutput().getWritten())) + !getResponse().isContentComplete(_servletContextRequest.getHttpOutput().getWritten())) { if (sendErrorOrAbort("Insufficient content written")) break; @@ -625,21 +631,21 @@ private void dispatch(DispatcherType type, Dispatchable dispatchable) throws Exc { try { - _request.getResponse().getHttpOutput().reopen(); - _servletContextApi.getContext().getServletContextHandler().requestInitialized(_request, _request.getHttpServletRequest()); + _servletContextRequest.getResponse().getHttpOutput().reopen(); + _servletContextApi.getContext().getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); getHttpOutput().reopen(); - _combinedListener.onBeforeDispatch(_request); + _combinedListener.onBeforeDispatch(_servletContextRequest); dispatchable.dispatch(); } catch (Throwable x) { - _combinedListener.onDispatchFailure(_request, x); + _combinedListener.onDispatchFailure(_servletContextRequest, x); throw x; } finally { - _combinedListener.onAfterDispatch(_request); - _servletContextApi.getContext().getServletContextHandler().requestDestroyed(_request, _request.getHttpServletRequest()); + _combinedListener.onAfterDispatch(_servletContextRequest); + _servletContextApi.getContext().getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); } } @@ -661,19 +667,19 @@ protected void handleException(Throwable failure) if (quiet != null || !getServer().isRunning()) { if (LOG.isDebugEnabled()) - LOG.debug(_request.getHttpServletRequest().getRequestURI(), failure); + LOG.debug(_servletContextRequest.getHttpServletRequest().getRequestURI(), failure); } else if (noStack != null) { // No stack trace unless there is debug turned on if (LOG.isDebugEnabled()) - LOG.warn("handleException {}", _request.getHttpServletRequest().getRequestURI(), failure); + LOG.warn("handleException {}", _servletContextRequest.getHttpServletRequest().getRequestURI(), failure); else - LOG.warn("handleException {} {}", _request.getHttpServletRequest().getRequestURI(), noStack.toString()); + LOG.warn("handleException {} {}", _servletContextRequest.getHttpServletRequest().getRequestURI(), noStack.toString()); } else { - LOG.warn(_request.getHttpServletRequest().getRequestURI(), failure); + LOG.warn(_servletContextRequest.getHttpServletRequest().getRequestURI(), failure); } if (isCommitted()) @@ -740,14 +746,14 @@ public boolean isExpecting102Processing() @Override public String toString() { - if (_request == null) + if (_servletContextRequest == null) { return String.format("%s@%x{null}", getClass().getSimpleName(), hashCode()); } - long timeStamp = _request.getTimeStamp(); + long timeStamp = _servletContextRequest.getTimeStamp(); return String.format("%s@%x{s=%s,r=%s,c=%b/%b,a=%s,uri=%s,age=%d}", getClass().getSimpleName(), hashCode(), @@ -756,7 +762,7 @@ public String toString() isRequestCompleted(), isResponseCompleted(), _state.getState(), - _request.getHttpURI(), + _servletContextRequest.getHttpURI(), timeStamp == 0 ? 0 : System.currentTimeMillis() - timeStamp); } @@ -776,13 +782,13 @@ protected boolean checkAndPrepareUpgrade() void onTrailers(HttpFields trailers) { - _request.setTrailers(trailers); - _combinedListener.onRequestTrailers(_request); + _servletContextRequest.setTrailers(trailers); + _combinedListener.onRequestTrailers(_servletContextRequest); } public void onCompleted() { - ServletContextRequest.ServletApiRequest apiRequest = _request.getServletApiRequest(); + ServletContextRequest.ServletApiRequest apiRequest = _servletContextRequest.getServletApiRequest(); if (LOG.isDebugEnabled()) LOG.debug("onCompleted for {} written={}", apiRequest.getRequestURI(), getBytesWritten()); @@ -794,19 +800,19 @@ public void onCompleted() { Authentication authentication = apiRequest.getAuthentication(); if (authentication instanceof Authentication.User userAuthentication) - _request.setAttribute(CustomRequestLog.USER_NAME, userAuthentication.getUserIdentity().getUserPrincipal().getName()); + _servletContextRequest.setAttribute(CustomRequestLog.USER_NAME, userAuthentication.getUserIdentity().getUserPrincipal().getName()); - String realPath = apiRequest.getServletContext().getRealPath(Request.getPathInContext(_request)); - _request.setAttribute(CustomRequestLog.REAL_PATH, realPath); + String realPath = apiRequest.getServletContext().getRealPath(Request.getPathInContext(_servletContextRequest)); + _servletContextRequest.setAttribute(CustomRequestLog.REAL_PATH, realPath); - String servletName = _request.getServletName(); - _request.setAttribute(CustomRequestLog.HANDLER_NAME, servletName); + String servletName = _servletContextRequest.getServletName(); + _servletContextRequest.setAttribute(CustomRequestLog.HANDLER_NAME, servletName); } // Callback will either be succeeded here or failed in abort(). if (_state.completeResponse()) _callback.succeeded(); - _combinedListener.onComplete(_request); + _combinedListener.onComplete(_servletContextRequest); } public boolean isCommitted() @@ -848,7 +854,7 @@ public void abort(Throwable failure) { if (LOG.isDebugEnabled()) LOG.debug("abort {}", this, failure); - _combinedListener.onResponseFailure(_request, failure); + _combinedListener.onResponseFailure(_servletContextRequest, failure); _callback.failed(failure); } } @@ -1032,10 +1038,10 @@ private class Listeners implements Listener { private final List _listeners; - private Listeners(ServletContextRequest request) + private Listeners(Connector connector, ServletContextHandler servletContextHandler) { - Collection connectorListeners = request.getConnectionMetaData().getConnector().getBeans(Listener.class); - List handlerListeners = request.getContext().getServletContextHandler().getEventListeners().stream() + Collection connectorListeners = connector.getBeans(Listener.class); + List handlerListeners = servletContextHandler.getEventListeners().stream() .filter(l -> l instanceof Listener) .map(Listener.class::cast) .toList(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index a124d5c083b4..b24287c4ff45 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -1198,14 +1198,11 @@ protected ServletContextRequest wrap(Request request) return null; // Get a servlet request, possibly from a cached version in the channel attributes. - // TODO We should cache this heavy weight object! Something like: - // TODO: ServletChannel is not properly cleared out so I have disabled the caching of this for now. - ServletChannel servletChannel = null; // (ServletChannel)request.getComponents().getCache().get("blah.blah.ServletChannel"); + ServletChannel servletChannel = null; // (ServletChannel)request.getComponents().getCache().get(ServletChannel.class.getName()); if (servletChannel == null) { - // TODO this may not be the right object to recycle, but ultimately we want to reuse: HttpInput, HttpOutput, ServletChannelState etc. etc. - servletChannel = new ServletChannel(); - // request.getComponents().getCache().put("blah.blah.ServletChannel", servletChannel); TODO: Re-enable. + servletChannel = new ServletChannel(this, request); + request.getComponents().getCache().put(ServletChannel.class.getName(), servletChannel); } ServletContextRequest servletContextRequest = new ServletContextRequest(_servletContext, servletChannel, request, pathInContext, diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java index 748d5703c78f..af33f6cc1e15 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java @@ -104,7 +104,7 @@ public static ServletContextRequest getServletContextRequest(ServletRequest requ Object channel = request.getAttribute(ServletChannel.class.getName()); if (channel instanceof ServletChannel) - return ((ServletChannel)channel).getRequest(); + return ((ServletChannel)channel).getServletContextRequest(); while (request instanceof ServletRequestWrapper) { @@ -142,7 +142,7 @@ protected ServletContextRequest( _servletChannel = servletChannel; _httpServletRequest = new ServletApiRequest(); _mappedServlet = mappedServlet; - _httpInput = new HttpInput(_servletChannel); // TODO recycle + _httpInput = _servletChannel.getHttpInput(); _pathInContext = pathInContext; _pathSpec = pathSpec; _matchedPath = matchedPath; diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextResponse.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextResponse.java index 9863184d543c..6be24165c37b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextResponse.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextResponse.java @@ -365,7 +365,7 @@ public String getCharacterEncoding(boolean setContentType) } // Try any default char encoding for the context. - ServletContext context = _servletChannel.getRequest().getContext().getServletContext(); + ServletContext context = _servletChannel.getServletContextRequest().getContext().getServletContext(); if (context != null) { encoding = context.getResponseCharacterEncoding(); diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java index 435fa8909322..85a9296403ff 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java @@ -480,7 +480,7 @@ private Action nextAction(boolean handling) return Action.WRITE_CALLBACK; } - Scheduler scheduler = _servletChannel.getRequest() + Scheduler scheduler = _servletChannel.getServletContextRequest() .getConnectionMetaData().getConnector().getScheduler(); if (scheduler != null && _timeoutMs > 0 && !_event.hasTimeoutTask()) _event.setTimeoutTask(scheduler.schedule(_event, _timeoutMs, TimeUnit.MILLISECONDS)); @@ -859,7 +859,7 @@ private void sendError(Throwable th) // No sync as this is always called with lock held // Determine the actual details of the exception - final Request request = _servletChannel.getRequest(); + final Request request = _servletChannel.getServletContextRequest(); final int code; final String message; Throwable cause = _servletChannel.unwrap(th, BadMessageException.class, UnavailableException.class); @@ -910,10 +910,10 @@ public void sendError(int code, String message) // - after unhandle for sync // - after both unhandle and complete for async - ServletContextRequest servletContextRequest = _servletChannel.getRequest(); + ServletContextRequest servletContextRequest = _servletChannel.getServletContextRequest(); HttpServletRequest httpServletRequest = servletContextRequest.getHttpServletRequest(); - final Request request = _servletChannel.getRequest(); + final Request request = _servletChannel.getServletContextRequest(); final Response response = _servletChannel.getResponse(); if (message == null) message = HttpStatus.getMessage(code); @@ -1110,7 +1110,7 @@ public void upgrade() protected void scheduleDispatch() { // TODO long winded!!! - _servletChannel.getRequest().getConnectionMetaData().getConnector().getExecutor().execute(_servletChannel); + _servletChannel.getServletContextRequest().getConnectionMetaData().getConnector().getExecutor().execute(_servletChannel); } protected void cancelTimeout() @@ -1198,7 +1198,7 @@ public ServletResponse getServletResponse(AsyncContextEvent event) if (event != null && event.getSuppliedResponse() != null) return event.getSuppliedResponse(); - ServletContextRequest servletContextRequest = _servletChannel.getRequest(); + ServletContextRequest servletContextRequest = _servletChannel.getServletContextRequest(); if (servletContextRequest != null) return servletContextRequest.getHttpServletResponse(); return null; @@ -1211,17 +1211,17 @@ void runInContext(AsyncContextEvent event, Runnable runnable) public Object getAttribute(String name) { - return _servletChannel.getRequest().getAttribute(name); + return _servletChannel.getServletContextRequest().getAttribute(name); } public void removeAttribute(String name) { - _servletChannel.getRequest().removeAttribute(name); + _servletChannel.getServletContextRequest().removeAttribute(name); } public void setAttribute(String name, Object attribute) { - _servletChannel.getRequest().setAttribute(name, attribute); + _servletChannel.getServletContextRequest().setAttribute(name, attribute); } /** From 1624c5219cf7a7dfbcfb7bf2d9e308fc40e6f6c9 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Thu, 17 Nov 2022 16:52:12 +1100 Subject: [PATCH 02/12] Recycle ServletChannel and resolve associated TODOs --- .../java/org/eclipse/jetty/ee10/servlet/HttpInput.java | 1 - .../org/eclipse/jetty/ee10/servlet/ServletChannel.java | 9 +++++++-- .../jetty/ee10/servlet/ServletContextHandler.java | 2 +- .../eclipse/jetty/ee10/servlet/AsyncServletIOTest.java | 1 + 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index 68cec4e7edb9..2a1d48a24d86 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -70,7 +70,6 @@ public void recycle() public void reopen() { - // TODO why is this never used? try (AutoLock lock = _lock.lock()) { if (LOG.isDebugEnabled()) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index b58637f9c5bc..371f3eb8a45b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -101,10 +101,11 @@ public void setCallback(Callback callback) public void init(ServletContextRequest servletContextRequest) { _state.recycle(); - _httpInput.recycle(); + _httpInput.reopen(); _servletContextApi = servletContextRequest.getContext().getServletContext(); _servletContextRequest = servletContextRequest; _expects100Continue = servletContextRequest.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()); + _callback = null; if (LOG.isDebugEnabled()) LOG.debug("new {} -> {},{}", @@ -351,10 +352,12 @@ public void continue100(int available) throws IOException } } - public void recycle() + private void recycle() { _written = 0; _oldIdleTimeout = 0; + _httpInput.recycle(); + _callback = null; } @Override @@ -813,6 +816,8 @@ public void onCompleted() if (_state.completeResponse()) _callback.succeeded(); _combinedListener.onComplete(_servletContextRequest); + + recycle(); } public boolean isCommitted() diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index b24287c4ff45..9c80b9786f7e 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -1198,7 +1198,7 @@ protected ServletContextRequest wrap(Request request) return null; // Get a servlet request, possibly from a cached version in the channel attributes. - ServletChannel servletChannel = null; // (ServletChannel)request.getComponents().getCache().get(ServletChannel.class.getName()); + ServletChannel servletChannel = (ServletChannel)request.getComponents().getCache().get(ServletChannel.class.getName()); if (servletChannel == null) { servletChannel = new ServletChannel(this, request); diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java index 25b48c67881c..5b0986880117 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java @@ -416,6 +416,7 @@ public void doGet(final HttpServletRequest request, final HttpServletResponse re @Override public void onError(Throwable t) { + t.printStackTrace(); if (complete.decrementAndGet() == 0) async.complete(); } From 083cfd6f52e0a2b5737e73d75cfa721e2d1ecb6a Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 18 Nov 2022 08:41:20 +1100 Subject: [PATCH 03/12] Cleanup caching comments and impl --- .../org/eclipse/jetty/server/Components.java | 14 ++--- .../org/eclipse/jetty/server/Request.java | 10 ++-- .../server/internal/HttpChannelState.java | 8 +-- .../org/eclipse/jetty/util/Attributes.java | 52 +++++++++++++++++-- .../ee10/servlet/ServletContextHandler.java | 6 ++- .../jetty/ee9/nested/ContextHandler.java | 4 +- 6 files changed, 72 insertions(+), 22 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java index e2d6ddad97ad..1d170dcb442d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java @@ -13,9 +13,8 @@ package org.eclipse.jetty.server; -import java.util.Map; - import org.eclipse.jetty.io.ByteBufferPool; +import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.thread.Scheduler; import org.eclipse.jetty.util.thread.ThreadPool; @@ -36,12 +35,13 @@ public interface Components * The cache will have a life cycle limited by the connection, i.e. no cache map will live * longer that the connection associated with it. However, a cache may have a shorter life * than a connection (e.g. it may be discarded for implementation reasons). A cache map is - * guaranteed to be give to only a single request concurrently, so objects saved there do not + * guaranteed to be given to only a single request concurrently (scoped by + * {@link org.eclipse.jetty.server.internal.HttpChannelState}), so objects saved there do not * need to be made safe from access by simultaneous request. - * If the connection is known to be none-persistent then the cache may be a noop cache and discard - * all items set on it. + * If the connection is known to be none-persistent then the cache may be a noop + * cache and discard all items set on it. + * * @return A Map, which may be an empty map that discards all items. - * TODO This should be Attributes like everything else. */ - Map getCache(); + Attributes getCache(); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index c385cd787c75..5ee014c8be39 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -117,6 +117,8 @@ public interface Request extends Attributes, Content.Source { List __defaultLocale = Collections.singletonList(Locale.getDefault()); + String CACHE_ATTRIBUTE = Request.class.getCanonicalName() + ".CookieCache"; + String COOKIE_ATTRIBUTE = Request.class.getCanonicalName() + ".Cookies"; /** * an ID unique within the lifetime scope of the {@link ConnectionMetaData#getId()}). @@ -384,21 +386,21 @@ static Fields extractQueryParameters(Request request, Charset charset) static List getCookies(Request request) { // TODO modify Request and HttpChannel to be optimised for the known attributes - List cookies = (List)request.getAttribute(Request.class.getCanonicalName() + ".Cookies"); + List cookies = (List)request.getAttribute(COOKIE_ATTRIBUTE); if (cookies != null) return cookies; // TODO: review whether to store the cookie cache at the connection level, or whether to cache them at all. - CookieCache cookieCache = (CookieCache)request.getComponents().getCache().get(Request.class.getCanonicalName() + ".CookieCache"); + CookieCache cookieCache = (CookieCache)request.getComponents().getCache().getAttribute(CACHE_ATTRIBUTE); if (cookieCache == null) { // TODO compliance listeners? cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), null); - request.getComponents().getCache().put(Request.class.getCanonicalName() + ".CookieCache", cookieCache); + request.getComponents().getCache().setAttribute(CACHE_ATTRIBUTE, cookieCache); } cookies = cookieCache.getCookies(request.getHeaders()); - request.setAttribute(Request.class.getCanonicalName() + ".Cookies", cookies); + request.setAttribute(COOKIE_ATTRIBUTE, cookies); return cookies; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index 5a391bb2dcdb..0ae12a751329 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -169,7 +169,7 @@ enum WriteState private Callback _writeCallback; private Content.Chunk.Error _error; private Predicate _onError; - private Map _cache; + private Attributes _cache; public HttpChannelState(ConnectionMetaData connectionMetaData) { @@ -321,14 +321,14 @@ public ThreadPool getThreadPool() } @Override - public Map getCache() + public Attributes getCache() { if (_cache == null) { if (getConnectionMetaData().isPersistent()) - _cache = new HashMap<>(); + _cache = new Attributes.Mapped(new HashMap<>()); else - _cache = NULL_CACHE; + _cache = Attributes.NULL; } return _cache; } diff --git a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Attributes.java b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Attributes.java index b2ba83379abd..350837a87fbc 100644 --- a/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Attributes.java +++ b/jetty-core/jetty-util/src/main/java/org/eclipse/jetty/util/Attributes.java @@ -125,7 +125,6 @@ public int size() }; } - /** * Clear all attribute names */ @@ -225,6 +224,7 @@ public boolean equals(Object obj) { return getWrapped().equals(obj); } + } /** @@ -232,15 +232,23 @@ public boolean equals(Object obj) */ class Mapped implements Attributes { - private final java.util.concurrent.ConcurrentMap _map = new ConcurrentHashMap<>(); - private final Set _names = Collections.unmodifiableSet(_map.keySet()); + private final Map _map; + private final Set _names; public Mapped() { + this(new ConcurrentHashMap<>()); + } + + public Mapped(Map map) + { + _map = Objects.requireNonNull(map); + _names = Collections.unmodifiableSet(_map.keySet()); } public Mapped(Mapped attributes) { + this(); _map.putAll(attributes._map); } @@ -567,4 +575,42 @@ public boolean equals(Object o) return false; } } + + Attributes NULL = new Attributes() + { + @Override + public Object removeAttribute(String name) + { + return null; + } + + @Override + public Object setAttribute(String name, Object attribute) + { + return null; + } + + @Override + public Object getAttribute(String name) + { + return null; + } + + @Override + public Set getAttributeNameSet() + { + return Collections.emptySet(); + } + + @Override + public void clearAttributes() + { + } + + @Override + public Map asAttributeMap() + { + return Collections.emptyMap(); + } + }; } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index d1393d0c05df..3ba8a1434983 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -81,6 +81,7 @@ import org.eclipse.jetty.server.handler.ContextHandler; import org.eclipse.jetty.server.handler.ContextHandlerCollection; import org.eclipse.jetty.server.handler.ContextRequest; +import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.DecoratedObjectFactory; import org.eclipse.jetty.util.DeprecationWarning; import org.eclipse.jetty.util.ExceptionUtil; @@ -1180,11 +1181,12 @@ protected ServletContextRequest wrap(Request request) return null; // Get a servlet request, possibly from a cached version in the channel attributes. - ServletChannel servletChannel = (ServletChannel)request.getComponents().getCache().get(ServletChannel.class.getName()); + Attributes cache = request.getComponents().getCache(); + ServletChannel servletChannel = (ServletChannel)cache.getAttribute(ServletChannel.class.getName()); if (servletChannel == null) { servletChannel = new ServletChannel(this, request); - request.getComponents().getCache().put(ServletChannel.class.getName(), servletChannel); + cache.setAttribute(ServletChannel.class.getName(), servletChannel); } ServletContextRequest servletContextRequest = new ServletContextRequest(_servletContext, servletChannel, request, pathInContext, diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java index aa010fc9ae3e..c152171cdcf5 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/ContextHandler.java @@ -2450,11 +2450,11 @@ protected CoreContext newContext() @Override protected ContextRequest wrap(org.eclipse.jetty.server.Request request) { - HttpChannel httpChannel = (HttpChannel)request.getComponents().getCache().get(HttpChannel.class.getName()); + HttpChannel httpChannel = (HttpChannel)request.getComponents().getCache().getAttribute(HttpChannel.class.getName()); if (httpChannel == null) { httpChannel = new HttpChannel(ContextHandler.this, request.getConnectionMetaData()); - request.getComponents().getCache().put(HttpChannel.class.getName(), httpChannel); + request.getComponents().getCache().setAttribute(HttpChannel.class.getName(), httpChannel); } else if (httpChannel.getContextHandler() == ContextHandler.this) { From 7299be05560a38f161925e7b30638ea2faebcc47 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Fri, 18 Nov 2022 09:29:18 +1100 Subject: [PATCH 04/12] Removed TODO --- .../src/main/java/org/eclipse/jetty/server/Components.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java index 1d170dcb442d..e3ef0ec52d68 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java @@ -19,8 +19,7 @@ import org.eclipse.jetty.util.thread.ThreadPool; /** - * Components made available via a {@link Request} - * TODO flesh out this idea... maybe better name? + * Common components made available via a {@link Request} */ public interface Components { From 0afbd11fb8a4b57d0bc4846389a92fc415851ce7 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 21 Nov 2022 09:26:10 +1100 Subject: [PATCH 05/12] Don't recycle after completion notification --- .../eclipse/jetty/ee10/servlet/HttpInput.java | 2 +- .../eclipse/jetty/ee10/servlet/HttpOutput.java | 4 ++-- .../jetty/ee10/servlet/ServletChannel.java | 17 +++++++---------- .../jetty/ee10/servlet/ServletRequestState.java | 6 +++--- 4 files changed, 13 insertions(+), 16 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index 2a1d48a24d86..793354ba6bf6 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -329,7 +329,7 @@ private int read(ByteBuffer buffer, byte[] b, int off, int len) throws IOExcepti private void scheduleReadListenerNotification() { - _servletChannel.execute(_servletChannel); + _servletChannel.execute(_servletChannel::handle); } /** diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index 60b8c102e9a2..6f8ab288e0c0 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -354,7 +354,7 @@ else if (closeContent != null) finally { if (wake) - _servletChannel.execute(_servletChannel); // TODO review in jetty-10 if execute is needed + _servletChannel.execute(_servletChannel::handle); // TODO review in jetty-10 if execute is needed } } @@ -1390,7 +1390,7 @@ public void setWriteListener(WriteListener writeListener) wake = _servletChannel.getState().onWritePossible(); } if (wake) - _servletChannel.execute(_servletChannel); + _servletChannel.execute(_servletChannel::handle); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 371f3eb8a45b..1402a7981f48 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -60,7 +60,7 @@ /** * TODO describe what this class does */ -public class ServletChannel implements Runnable +public class ServletChannel { private static final Logger LOG = LoggerFactory.getLogger(ServletChannel.class); @@ -360,12 +360,6 @@ private void recycle() _callback = null; } - @Override - public void run() - { - handle(); - } - /** * @return True if the channel is ready to continue handling (ie it is not suspended) */ @@ -814,10 +808,13 @@ public void onCompleted() // Callback will either be succeeded here or failed in abort(). if (_state.completeResponse()) - _callback.succeeded(); + { + Callback callback = _callback; + // Must recycle before notification to allow for reuse. + recycle(); + callback.succeeded(); + } _combinedListener.onComplete(_servletContextRequest); - - recycle(); } public boolean isCommitted() diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java index 85a9296403ff..72d91294bf50 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletRequestState.java @@ -733,7 +733,7 @@ public void complete() cancelTimeout(event); if (handle) - runInContext(event, _servletChannel); + runInContext(event, _servletChannel::handle); } public void asyncError(Throwable failure) @@ -767,7 +767,7 @@ public void asyncError(Throwable failure) if (event != null) { cancelTimeout(event); - runInContext(event, _servletChannel); + runInContext(event, _servletChannel::handle); } } @@ -1110,7 +1110,7 @@ public void upgrade() protected void scheduleDispatch() { // TODO long winded!!! - _servletChannel.getServletContextRequest().getConnectionMetaData().getConnector().getExecutor().execute(_servletChannel); + _servletChannel.getServletContextRequest().getConnectionMetaData().getConnector().getExecutor().execute(_servletChannel::handle); } protected void cancelTimeout() From 262d53014c2b99cc796286f7192c2ab8056d8c82 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 21 Nov 2022 09:37:59 +1100 Subject: [PATCH 06/12] cleanup tests --- .../eclipse/jetty/ee10/servlet/ServletChannel.java | 10 ++++++++-- .../eclipse/jetty/ee10/servlet/ErrorPageTest.java | 12 +++++------- 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 1402a7981f48..908558b075e1 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -783,6 +783,9 @@ void onTrailers(HttpFields trailers) _combinedListener.onRequestTrailers(_servletContextRequest); } + /** + * @see #abort(Throwable) + */ public void onCompleted() { ServletContextRequest.ServletApiRequest apiRequest = _servletContextRequest.getServletApiRequest(); @@ -812,9 +815,9 @@ public void onCompleted() Callback callback = _callback; // Must recycle before notification to allow for reuse. recycle(); + _combinedListener.onComplete(_servletContextRequest); callback.succeeded(); } - _combinedListener.onComplete(_servletContextRequest); } public boolean isCommitted() @@ -848,6 +851,7 @@ protected void execute(Runnable task) * then this method should be called. * * @param failure the failure that caused the abort. + * @see #onCompleted() */ public void abort(Throwable failure) { @@ -856,8 +860,10 @@ public void abort(Throwable failure) { if (LOG.isDebugEnabled()) LOG.debug("abort {}", this, failure); + Callback callback = _callback; + recycle(); _combinedListener.onResponseFailure(_servletContextRequest, failure); - _callback.failed(failure); + callback.failed(failure); } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java index 5916c167a831..a7d7484cd6ee 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ErrorPageTest.java @@ -42,7 +42,6 @@ import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.Handler; -import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.Server; @@ -151,7 +150,6 @@ public void testErrorOverridesMimeTypeAndCharset() throws Exception rawRequest.append("\r\n"); String rawResponse = _connector.getResponse(rawRequest.toString()); - System.out.println(rawResponse); HttpTester.Response response = HttpTester.parseResponse(rawResponse); assertThat(response.getStatus(), is(595)); @@ -311,7 +309,7 @@ public void testErrorMessage() throws Exception public void testErrorException() throws Exception { _errorPageErrorHandler.setUnwrapServletException(false); - try (StacklessLogging stackless = new StacklessLogging(HttpChannel.class)) + try (StacklessLogging stackless = new StacklessLogging(ServletChannel.class)) { String response = _connector.getResponse("GET /fail/exception HTTP/1.0\r\n\r\n"); assertThat(response, Matchers.containsString("HTTP/1.1 500 Server Error")); @@ -332,7 +330,7 @@ public void testErrorException() throws Exception } _errorPageErrorHandler.setUnwrapServletException(true); - try (StacklessLogging stackless = new StacklessLogging(HttpChannel.class)) + try (StacklessLogging stackless = new StacklessLogging(ServletChannel.class)) { String response = _connector.getResponse("GET /fail/exception HTTP/1.0\r\n\r\n"); assertThat(response, Matchers.containsString("HTTP/1.1 500 Server Error")); @@ -369,7 +367,7 @@ public void testGlobalErrorCode() throws Exception @Test public void testGlobalErrorException() throws Exception { - try (StacklessLogging stackless = new StacklessLogging(HttpChannel.class)) + try (StacklessLogging stackless = new StacklessLogging(ServletChannel.class)) { String response = _connector.getResponse("GET /fail/global?code=NAN HTTP/1.0\r\n\r\n"); assertThat(response, Matchers.containsString("HTTP/1.1 500 Server Error")); @@ -494,7 +492,7 @@ public void testPermanentlyUnavailable() throws Exception { try (StacklessLogging ignore = new StacklessLogging(_context.getLogger())) { - try (StacklessLogging ignore2 = new StacklessLogging(HttpChannel.class)) + try (StacklessLogging ignore2 = new StacklessLogging(ServletChannel.class)) { __destroyed = new AtomicBoolean(false); String response = _connector.getResponse("GET /unavailable/info HTTP/1.0\r\n\r\n"); @@ -510,7 +508,7 @@ public void testUnavailable() throws Exception { try (StacklessLogging ignore = new StacklessLogging(_context.getLogger())) { - try (StacklessLogging ignore2 = new StacklessLogging(HttpChannel.class)) + try (StacklessLogging ignore2 = new StacklessLogging(ServletChannel.class)) { __destroyed = new AtomicBoolean(false); String response = _connector.getResponse("GET /unavailable/info?for=1 HTTP/1.0\r\n\r\n"); From c73133b6fbf309c11ea53209ddb0cfafbed09379 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 21 Nov 2022 09:56:04 +1100 Subject: [PATCH 07/12] updates from review --- .../jetty/ee10/servlet/ServletChannel.java | 17 +++++++++++++---- .../ee10/servlet/ServletContextHandler.java | 2 +- .../jetty/ee10/servlet/AsyncServletIOTest.java | 1 - 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index 908558b075e1..d24f2287de2a 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -98,14 +98,18 @@ public void setCallback(Callback callback) _callback = callback; } - public void init(ServletContextRequest servletContextRequest) + /** + * Associate this channel with a specific request. + * @param servletContextRequest The request to associate + * @see #recycle() + */ + public void associate(ServletContextRequest servletContextRequest) { _state.recycle(); _httpInput.reopen(); _servletContextApi = servletContextRequest.getContext().getServletContext(); _servletContextRequest = servletContextRequest; _expects100Continue = servletContextRequest.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()); - _callback = null; if (LOG.isDebugEnabled()) LOG.debug("new {} -> {},{}", @@ -352,12 +356,17 @@ public void continue100(int available) throws IOException } } + /** + * @see #associate(ServletContextRequest) + */ private void recycle() { - _written = 0; - _oldIdleTimeout = 0; _httpInput.recycle(); + _servletContextApi = null; + _servletContextRequest = null; _callback = null; + _written = 0; + _oldIdleTimeout = 0; } /** diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index 3ba8a1434983..1faedac09551 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -1191,7 +1191,7 @@ protected ServletContextRequest wrap(Request request) ServletContextRequest servletContextRequest = new ServletContextRequest(_servletContext, servletChannel, request, pathInContext, matchedResource.getResource(), matchedResource.getPathSpec(), matchedResource.getMatchedPath()); - servletChannel.init(servletContextRequest); + servletChannel.associate(servletContextRequest); return servletContextRequest; } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java index 5b0986880117..25b48c67881c 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/AsyncServletIOTest.java @@ -416,7 +416,6 @@ public void doGet(final HttpServletRequest request, final HttpServletResponse re @Override public void onError(Throwable t) { - t.printStackTrace(); if (complete.decrementAndGet() == 0) async.complete(); } From 4c13604632052b7bd605e7532cf71415ee0bc04f Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 21 Nov 2022 13:39:35 +1100 Subject: [PATCH 08/12] Delay setting callback until ServletHandler.handle called --- .../eclipse/jetty/ee10/servlet/ServletContextRequest.java | 8 -------- .../org/eclipse/jetty/ee10/servlet/ServletHandler.java | 1 + 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java index af33f6cc1e15..aa57a7b600e1 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextRequest.java @@ -78,7 +78,6 @@ import org.eclipse.jetty.server.handler.ContextResponse; import org.eclipse.jetty.session.Session; import org.eclipse.jetty.session.SessionManager; -import org.eclipse.jetty.util.Callback; import org.eclipse.jetty.util.Fields; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.StringUtil; @@ -153,13 +152,6 @@ public String getPathInContext() return _pathInContext; } - @Override - public void process(Request request, Response response, Callback callback) throws Exception - { - _servletChannel.setCallback(callback); - super.process(request, response, callback); - } - @Override public HttpFields getTrailers() { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java index e8f8933272ba..6a63c1d4a925 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletHandler.java @@ -438,6 +438,7 @@ public Request.Processor handle(Request request) throws Exception { // We will always have a ServletScopedRequest and MappedServlet otherwise we will not reach ServletHandler. ServletContextRequest servletRequest = Request.as(request, ServletContextRequest.class); + servletRequest.getServletChannel().setCallback(cb); servletRequest.getServletChannel().handle(); }; } From 577d2372fbcd3cb36fec61cf33ce01fe832864bd Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Mon, 21 Nov 2022 16:13:09 +1100 Subject: [PATCH 09/12] Improved recycle lifecycle --- .../jetty/ee10/servlet/ServletChannel.java | 53 ++++++++++--------- .../ee10/servlets/EventSourceServlet.java | 13 +++-- 2 files changed, 37 insertions(+), 29 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index d24f2287de2a..d646f15b6f52 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -65,6 +65,8 @@ public class ServletChannel private static final Logger LOG = LoggerFactory.getLogger(ServletChannel.class); private final ServletRequestState _state; + private final ServletContextHandler.Context _context; + private final ServletContextHandler.ServletContextApi _servletContextApi; private final AtomicLong _requests = new AtomicLong(); private final Connector _connector; private final Executor _executor; @@ -72,17 +74,18 @@ public class ServletChannel private final EndPoint _endPoint; private final HttpInput _httpInput; private final Listener _combinedListener; - private ServletContextHandler.ServletContextApi _servletContextApi; - private ServletContextRequest _servletContextRequest; - private boolean _expects100Continue; - private long _oldIdleTimeout; - private Callback _callback; + private volatile ServletContextRequest _servletContextRequest; + private volatile boolean _expects100Continue; + private volatile long _oldIdleTimeout; + private volatile Callback _callback; // Bytes written after interception (e.g. after compression). - private long _written; + private volatile long _written; public ServletChannel(ServletContextHandler servletContextHandler, Request request) { _state = new ServletRequestState(this); + _context = servletContextHandler.getContext(); + _servletContextApi = _context.getServletContext(); _connector = request.getConnectionMetaData().getConnector(); _executor = request.getContext(); _configuration = request.getConnectionMetaData().getHttpConfiguration(); @@ -107,7 +110,6 @@ public void associate(ServletContextRequest servletContextRequest) { _state.recycle(); _httpInput.reopen(); - _servletContextApi = servletContextRequest.getContext().getServletContext(); _servletContextRequest = servletContextRequest; _expects100Continue = servletContextRequest.getHeaders().contains(HttpHeader.EXPECT, HttpHeaderValue.CONTINUE.asString()); @@ -120,12 +122,12 @@ public void associate(ServletContextRequest servletContextRequest) public ServletContextHandler.Context getContext() { - return _servletContextRequest.getContext(); + return _context; } public ServletContextHandler getContextHandler() { - return getContext().getContextHandler(); + return _context.getContextHandler(); } public ServletContextHandler.ServletContextApi getServletContext() @@ -213,7 +215,8 @@ public ServletContextRequest getServletContextRequest() public ServletContextResponse getResponse() { - return _servletContextRequest.getResponse(); + ServletContextRequest request = _servletContextRequest; + return request == null ? null : request.getResponse(); } public Connection getConnection() @@ -362,7 +365,6 @@ public void continue100(int available) throws IOException private void recycle() { _httpInput.recycle(); - _servletContextApi = null; _servletContextRequest = null; _callback = null; _written = 0; @@ -406,7 +408,7 @@ public boolean handle() dispatch(DispatcherType.REQUEST, () -> { ServletContextHandler.ServletContextApi servletContextApi = getServletContextContext(); - ServletHandler servletHandler = servletContextApi.getContext().getServletContextHandler().getServletHandler(); + ServletHandler servletHandler = _context.getServletContextHandler().getServletHandler(); ServletHandler.MappedServlet mappedServlet = _servletContextRequest._mappedServlet; mappedServlet.handle(servletHandler, Request.getPathInContext(_servletContextRequest), _servletContextRequest.getHttpServletRequest(), _servletContextRequest.getHttpServletResponse()); @@ -425,7 +427,7 @@ public boolean handle() String dispatchString = asyncContextEvent.getDispatchPath(); if (dispatchString != null) { - String contextPath = _servletContextRequest.getContext().getContextPath(); + String contextPath = _context.getContextPath(); HttpURI.Immutable dispatchUri = HttpURI.from(dispatchString); pathInContext = URIUtil.canonicalPath(dispatchUri.getPath()); uri = HttpURI.build(_servletContextRequest.getHttpURI()) @@ -442,8 +444,8 @@ public boolean handle() else { pathInContext = uri.getCanonicalPath(); - if (_servletContextRequest.getContext().getContextPath().length() > 1) - pathInContext = pathInContext.substring(_servletContextRequest.getContext().getContextPath().length()); + if (_context.getContextPath().length() > 1) + pathInContext = pathInContext.substring(_context.getContextPath().length()); } } @@ -534,15 +536,13 @@ public boolean handle() case READ_CALLBACK: { - ServletContextHandler handler = _state.getContextHandler(); - handler.getContext().run(() -> _servletContextRequest.getHttpInput().run()); + _context.run(() -> _servletContextRequest.getHttpInput().run()); break; } case WRITE_CALLBACK: { - ServletContextHandler handler = _state.getContextHandler(); - handler.getContext().run(() -> _servletContextRequest.getHttpOutput().run()); + _context.run(() -> _servletContextRequest.getHttpOutput().run()); break; } @@ -638,7 +638,7 @@ private void dispatch(DispatcherType type, Dispatchable dispatchable) throws Exc try { _servletContextRequest.getResponse().getHttpOutput().reopen(); - _servletContextApi.getContext().getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); + _context.getServletContextHandler().requestInitialized(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); getHttpOutput().reopen(); _combinedListener.onBeforeDispatch(_servletContextRequest); dispatchable.dispatch(); @@ -651,7 +651,7 @@ private void dispatch(DispatcherType type, Dispatchable dispatchable) throws Exc finally { _combinedListener.onAfterDispatch(_servletContextRequest); - _servletContextApi.getContext().getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); + _context.getServletContextHandler().requestDestroyed(_servletContextRequest, _servletContextRequest.getHttpServletRequest()); } } @@ -685,7 +685,8 @@ else if (noStack != null) } else { - LOG.warn(_servletContextRequest.getHttpServletRequest().getRequestURI(), failure); + ServletContextRequest request = _servletContextRequest; + LOG.warn(request == null ? "unknown request" : request.getHttpServletRequest().getRequestURI(), failure); } if (isCommitted()) @@ -819,11 +820,12 @@ public void onCompleted() } // Callback will either be succeeded here or failed in abort(). + Callback callback = _callback; + // Must recycle before notification to allow for reuse. + // Recycle always done here even if an abort is called. + recycle(); if (_state.completeResponse()) { - Callback callback = _callback; - // Must recycle before notification to allow for reuse. - recycle(); _combinedListener.onComplete(_servletContextRequest); callback.succeeded(); } @@ -870,7 +872,6 @@ public void abort(Throwable failure) if (LOG.isDebugEnabled()) LOG.debug("abort {}", this, failure); Callback callback = _callback; - recycle(); _combinedListener.onResponseFailure(_servletContextRequest, failure); callback.failed(failure); } diff --git a/jetty-ee10/jetty-ee10-servlets/src/main/java/org/eclipse/jetty/ee10/servlets/EventSourceServlet.java b/jetty-ee10/jetty-ee10-servlets/src/main/java/org/eclipse/jetty/ee10/servlets/EventSourceServlet.java index ae6b23ad22c2..c5b9648da308 100644 --- a/jetty-ee10/jetty-ee10-servlets/src/main/java/org/eclipse/jetty/ee10/servlets/EventSourceServlet.java +++ b/jetty-ee10/jetty-ee10-servlets/src/main/java/org/eclipse/jetty/ee10/servlets/EventSourceServlet.java @@ -201,9 +201,16 @@ public void run() } catch (IOException x) { - // The other peer closed the connection - close(); - eventSource.onClose(); + try + { + // The other peer closed the connection + close(); + eventSource.onClose(); + } + catch (Throwable t) + { + t.printStackTrace(); + } } } From cf9f02f0cd5a53c3d3d28d33941721495830687a Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 22 Nov 2022 11:08:24 +1100 Subject: [PATCH 10/12] cleanup locking in content producers --- .../jetty/ee10/servlet/AsyncContentProducer.java | 14 ++++++-------- .../ee10/servlet/BlockingContentProducer.java | 16 +++++----------- .../jetty/ee10/servlet/ContentProducer.java | 8 -------- .../eclipse/jetty/ee10/servlet/HttpInput.java | 7 +++++-- 4 files changed, 16 insertions(+), 29 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java index 3a351ddfc772..0d9998c34e79 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/AsyncContentProducer.java @@ -38,7 +38,7 @@ class AsyncContentProducer implements ContentProducer private static final Logger LOG = LoggerFactory.getLogger(AsyncContentProducer.class); private static final Content.Chunk.Error RECYCLED_ERROR_CHUNK = Content.Chunk.from(new StaticException("ContentProducer has been recycled")); - private final AutoLock _lock; + final AutoLock _lock; private final ServletChannel _servletChannel; private HttpInput.Interceptor _interceptor; private Content.Chunk _rawChunk; @@ -47,18 +47,16 @@ class AsyncContentProducer implements ContentProducer private long _firstByteNanoTime = Long.MIN_VALUE; private long _rawBytesArrived; + /** + * @param servletChannel The ServletChannel to produce input from. + * @param lock The lock of the HttpInput, shared with this instance + */ AsyncContentProducer(ServletChannel servletChannel, AutoLock lock) { _servletChannel = servletChannel; _lock = lock; } - @Override - public AutoLock lock() - { - return _lock.lock(); - } - @Override public void recycle() { @@ -524,7 +522,7 @@ LockedSemaphore newLockedSemaphore() } /** - * A semaphore that assumes working under {@link AsyncContentProducer#lock()} scope. + * A semaphore that assumes working under the same locked scope. */ class LockedSemaphore { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java index 9c7238b96b9b..af95640a571d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/BlockingContentProducer.java @@ -14,7 +14,6 @@ package org.eclipse.jetty.ee10.servlet; import org.eclipse.jetty.io.Content; -import org.eclipse.jetty.util.thread.AutoLock; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -28,21 +27,16 @@ class BlockingContentProducer implements ContentProducer private final AsyncContentProducer _asyncContentProducer; private final AsyncContentProducer.LockedSemaphore _semaphore; - private final AutoLock _lock; - BlockingContentProducer(AsyncContentProducer delegate, AutoLock lock) + /** + * @param asyncContentProducer The {@link AsyncContentProducer} to block against. + */ + BlockingContentProducer(AsyncContentProducer asyncContentProducer) { - _asyncContentProducer = delegate; - _lock = lock; + _asyncContentProducer = asyncContentProducer; _semaphore = _asyncContentProducer.newLockedSemaphore(); } - @Override - public AutoLock lock() - { - return _lock.lock(); - } - @Override public void recycle() { diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ContentProducer.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ContentProducer.java index e0d1c2da792f..310006b8fee2 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ContentProducer.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ContentProducer.java @@ -15,20 +15,12 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.util.component.Destroyable; -import org.eclipse.jetty.util.thread.AutoLock; /** * ContentProducer is the bridge between {@link HttpInput} and {@link Content.Source}. */ public interface ContentProducer { - /** - * Lock this instance. The lock must be held before any of this instance's - * method can be called, and must be released afterward. - * @return the lock that is guarding this instance. - */ - AutoLock lock(); - /** * Clear the interceptor and call {@link Destroyable#destroy()} on it if it implements {@link Destroyable}. * A recycled {@link ContentProducer} will only produce special content with a non-null error until diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java index 793354ba6bf6..01be5921d7bf 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpInput.java @@ -37,7 +37,10 @@ public class HttpInput extends ServletInputStream implements Runnable { private static final Logger LOG = LoggerFactory.getLogger(HttpInput.class); - private final AutoLock _lock = new AutoLock(); + /** + * The lock shared with {@link AsyncContentProducer} and this class. + */ + final AutoLock _lock = new AutoLock(); private final ServletChannel _servletChannel; private final ServletRequestState _channelState; private final byte[] _oneByteBuffer = new byte[1]; @@ -53,7 +56,7 @@ public HttpInput(ServletChannel channel) _servletChannel = channel; _channelState = _servletChannel.getState(); _asyncContentProducer = new AsyncContentProducer(_servletChannel, _lock); - _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer, _lock); + _blockingContentProducer = new BlockingContentProducer(_asyncContentProducer); _contentProducer = _blockingContentProducer; } From fa72e785bc83a5bd93ba7ff455749f098e0538a3 Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 22 Nov 2022 11:32:18 +1100 Subject: [PATCH 11/12] updates from review --- .../org/eclipse/jetty/ee10/servlet/HttpOutput.java | 2 +- .../eclipse/jetty/ee10/servlet/ServletChannel.java | 13 +++++++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java index 6f8ab288e0c0..4e3954c53b2e 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/HttpOutput.java @@ -354,7 +354,7 @@ else if (closeContent != null) finally { if (wake) - _servletChannel.execute(_servletChannel::handle); // TODO review in jetty-10 if execute is needed + _servletChannel.execute(_servletChannel::handle); } } diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java index d646f15b6f52..2b18ddb70751 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletChannel.java @@ -58,7 +58,15 @@ import static org.eclipse.jetty.util.thread.Invocable.InvocationType.NON_BLOCKING; /** - * TODO describe what this class does + * The ServletChannel contains the state and behaviors associated with the Servlet API + * lifecycle for a single request/response cycle. Specifically it uses + * {@link ServletRequestState} to coordinate the states of dispatch state, input and + * output according to the servlet specification. The combined state so obtained + * is reflected in the behaviour of the contained {@link HttpInput} implementation of + * {@link jakarta.servlet.ServletInputStream}. + * + * @see ServletRequestState + * @see HttpInput */ public class ServletChannel { @@ -821,12 +829,13 @@ public void onCompleted() // Callback will either be succeeded here or failed in abort(). Callback callback = _callback; + ServletContextRequest servletContextRequest = _servletContextRequest; // Must recycle before notification to allow for reuse. // Recycle always done here even if an abort is called. recycle(); if (_state.completeResponse()) { - _combinedListener.onComplete(_servletContextRequest); + _combinedListener.onComplete(servletContextRequest); callback.succeeded(); } } From 2c9918818af71cd35bdb31b025bc20b460f6fddc Mon Sep 17 00:00:00 2001 From: Greg Wilkins Date: Tue, 22 Nov 2022 17:11:09 +1100 Subject: [PATCH 12/12] Check that the retrieved ServletChannel is for the same context. --- .../ee10/servlet/ServletContextHandler.java | 2 +- .../session/ClientCrossContextSessionTest.java | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java index 1faedac09551..64d2775f972e 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletContextHandler.java @@ -1183,7 +1183,7 @@ protected ServletContextRequest wrap(Request request) // Get a servlet request, possibly from a cached version in the channel attributes. Attributes cache = request.getComponents().getCache(); ServletChannel servletChannel = (ServletChannel)cache.getAttribute(ServletChannel.class.getName()); - if (servletChannel == null) + if (servletChannel == null || servletChannel.getContext() != getContext()) { servletChannel = new ServletChannel(this, request); cache.setAttribute(ServletChannel.class.getName(), servletChannel); diff --git a/jetty-ee10/test-ee10-sessions/test-ee10-sessions-common/src/test/java/org/eclipse/jetty/ee10/session/ClientCrossContextSessionTest.java b/jetty-ee10/test-ee10-sessions/test-ee10-sessions-common/src/test/java/org/eclipse/jetty/ee10/session/ClientCrossContextSessionTest.java index f50f3e8a4c3b..8914cde3cba6 100644 --- a/jetty-ee10/test-ee10-sessions/test-ee10-sessions-common/src/test/java/org/eclipse/jetty/ee10/session/ClientCrossContextSessionTest.java +++ b/jetty-ee10/test-ee10-sessions/test-ee10-sessions-common/src/test/java/org/eclipse/jetty/ee10/session/ClientCrossContextSessionTest.java @@ -51,7 +51,7 @@ public void testCrossContextDispatch() throws Exception { String contextA = "/contextA"; String contextB = "/contextB"; - String servletMapping = "/server"; + String servletPath = "/server"; DefaultSessionCacheFactory cacheFactory = new DefaultSessionCacheFactory(); cacheFactory.setEvictionPolicy(SessionCache.NEVER_EVICT); @@ -62,11 +62,11 @@ public void testCrossContextDispatch() throws Exception TestServletA servletA = new TestServletA(); ServletHolder holderA = new ServletHolder(servletA); ServletContextHandler ctxA = server.addContext(contextA); - ctxA.addServlet(holderA, servletMapping); + ctxA.addServlet(holderA, servletPath); ServletContextHandler ctxB = server.addContext(contextB); TestServletB servletB = new TestServletB(); ServletHolder holderB = new ServletHolder(servletB); - ctxB.addServlet(holderB, servletMapping); + ctxB.addServlet(holderB, servletPath); try { @@ -78,15 +78,15 @@ public void testCrossContextDispatch() throws Exception try { // Perform a request to contextA - ContentResponse response = client.GET("http://localhost:" + port + contextA + servletMapping); + ContentResponse responseA = client.GET("http://localhost:" + port + contextA + servletPath); - assertEquals(HttpServletResponse.SC_OK, response.getStatus()); - String sessionCookie = response.getHeaders().get("Set-Cookie"); + assertEquals(HttpServletResponse.SC_OK, responseA.getStatus()); + String sessionCookie = responseA.getHeaders().get("Set-Cookie"); assertNotNull(sessionCookie); String sessionId = SessionTestSupport.extractSessionId(sessionCookie); // Perform a request to contextB with the same session cookie - Request request = client.newRequest("http://localhost:" + port + contextB + servletMapping); + Request request = client.newRequest("http://localhost:" + port + contextB + servletPath); HttpField cookie = new HttpField("Cookie", "JSESSIONID=" + sessionId); request.headers(headers -> headers.put(cookie)); ContentResponse responseB = request.send(); @@ -131,7 +131,7 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t public static class TestServletB extends HttpServlet { private static final long serialVersionUID = 1L; - public String sessionId; + public volatile String sessionId; @Override protected void doGet(HttpServletRequest request, HttpServletResponse httpServletResponse) throws ServletException, IOException