From 955ca4d1462f40c23197c827bf20dea46c699830 Mon Sep 17 00:00:00 2001 From: Juergen Hoeller Date: Thu, 1 Dec 2022 19:45:40 +0100 Subject: [PATCH] Revised support for Jetty 12 (tested against 12.0.0.alpha2) Avoids HttpFields optimization completely, relying on Servlet header access instead. ServletServerHttpResponse provides applyHeaders/adaptHeaders split for better reuse. See gh-29575 --- .../reactive/AbstractServerHttpResponse.java | 4 +- .../server/reactive/JettyHeadersAdapter.java | 4 +- .../reactive/JettyHttpHandlerAdapter.java | 137 ++++++++---------- .../reactive/ServletHttpHandlerAdapter.java | 17 +-- .../reactive/ServletServerHttpResponse.java | 13 +- .../reactive/TomcatHttpHandlerAdapter.java | 29 +--- 6 files changed, 84 insertions(+), 120 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java index 89f343163a..91c52a45c4 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpResponse.java @@ -291,10 +291,10 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { /** * Invoked when the response is getting committed allowing subclasses to * make apply header values to the underlying response. - *

Note that most subclasses use an {@link HttpHeaders} instance that + *

Note that some subclasses use an {@link HttpHeaders} instance that * wraps an adapter to the native response headers such that changes are * propagated to the underlying response on the go. That means this callback - * is typically not used other than for specialized updates such as setting + * might not be used other than for specialized updates such as setting * the contentType or characterEncoding fields in a Servlet response. */ protected abstract void applyHeaders(); diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java index 63447a515b..e0dfecfd77 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHeadersAdapter.java @@ -45,8 +45,8 @@ class JettyHeadersAdapter implements MultiValueMap { private final HttpFields.Mutable headers; - JettyHeadersAdapter(HttpFields headers) { - this.headers = (headers instanceof HttpFields.Mutable mutable ? mutable : HttpFields.build(headers)); + JettyHeadersAdapter(HttpFields.Mutable headers) { + this.headers = headers; } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java index dc1e4f877a..694bf286f8 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/JettyHttpHandlerAdapter.java @@ -18,10 +18,8 @@ package org.springframework.http.server.reactive; import java.io.IOException; import java.io.OutputStream; -import java.lang.reflect.Method; import java.net.URISyntaxException; import java.nio.ByteBuffer; -import java.nio.charset.Charset; import jakarta.servlet.AsyncContext; import jakarta.servlet.http.HttpServletRequest; @@ -36,12 +34,9 @@ import org.eclipse.jetty.server.Response; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.HttpHeaders; -import org.springframework.http.MediaType; -import org.springframework.lang.Nullable; import org.springframework.util.Assert; import org.springframework.util.ClassUtils; import org.springframework.util.MultiValueMap; -import org.springframework.util.ReflectionUtils; /** * {@link ServletHttpHandlerAdapter} extension that uses Jetty APIs for writing @@ -49,6 +44,7 @@ import org.springframework.util.ReflectionUtils; * * @author Violeta Georgieva * @author Brian Clozel + * @author Juergen Hoeller * @since 5.0 * @see org.springframework.web.server.adapter.AbstractReactiveWebInitializer */ @@ -62,14 +58,6 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { "org.eclipse.jetty.ee10.servlet.HttpOutput", JettyHttpHandlerAdapter.class.getClassLoader()); */ - @Nullable - private static final Method getRequestHeadersMethod = - ClassUtils.getMethodIfAvailable(Request.class, "getHeaders"); - - @Nullable - private static final Method getResponseHeadersMethod = - ClassUtils.getMethodIfAvailable(Response.class, "getHeaders"); - public JettyHttpHandlerAdapter(HttpHandler httpHandler) { super(httpHandler); @@ -80,23 +68,39 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { protected ServletServerHttpRequest createRequest(HttpServletRequest request, AsyncContext context) throws IOException, URISyntaxException { - Assert.state(getServletPath() != null, "Servlet path is not initialized"); - return new JettyServerHttpRequest( - request, context, getServletPath(), getDataBufferFactory(), getBufferSize()); + if (jetty11Present) { + Assert.state(getServletPath() != null, "Servlet path is not initialized"); + return new Jetty11ServerHttpRequest( + request, context, getServletPath(), getDataBufferFactory(), getBufferSize()); + } + else { + return super.createRequest(request, context); + } } @Override protected ServletServerHttpResponse createResponse(HttpServletResponse response, AsyncContext context, ServletServerHttpRequest request) throws IOException { - return new JettyServerHttpResponse( - response, context, getDataBufferFactory(), getBufferSize(), request); + if (jetty11Present) { + return new Jetty11ServerHttpResponse( + response, context, getDataBufferFactory(), getBufferSize(), request); + } + /* Jetty 12: see spring-web.gradle + else if (jetty12Present) { + return new Jetty12ServerHttpResponse( + response, context, getDataBufferFactory(), getBufferSize(), request); + } + */ + else { + return super.createResponse(response, context, request); + } } - private static final class JettyServerHttpRequest extends ServletServerHttpRequest { + private static final class Jetty11ServerHttpRequest extends ServletServerHttpRequest { - JettyServerHttpRequest(HttpServletRequest request, AsyncContext asyncContext, + Jetty11ServerHttpRequest(HttpServletRequest request, AsyncContext asyncContext, String servletPath, DataBufferFactory bufferFactory, int bufferSize) throws IOException, URISyntaxException { @@ -105,14 +109,7 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { private static MultiValueMap createHeaders(HttpServletRequest servletRequest) { Request request = getRequest(servletRequest); - HttpFields fields; - if (getRequestHeadersMethod != null) { - fields = (HttpFields) ReflectionUtils.invokeMethod(getRequestHeadersMethod, request); - } - else { - fields = request.getHttpFields(); - } - return new JettyHeadersAdapter(fields); + return new JettyHeadersAdapter(HttpFields.build(request.getHttpFields())); } private static Request getRequest(HttpServletRequest request) { @@ -131,9 +128,9 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { } - private static final class JettyServerHttpResponse extends ServletServerHttpResponse { + private static final class Jetty11ServerHttpResponse extends ServletServerHttpResponse { - JettyServerHttpResponse(HttpServletResponse response, AsyncContext asyncContext, + Jetty11ServerHttpResponse(HttpServletResponse response, AsyncContext asyncContext, DataBufferFactory bufferFactory, int bufferSize, ServletServerHttpRequest request) throws IOException { @@ -142,14 +139,7 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { private static HttpHeaders createHeaders(HttpServletResponse servletResponse) { Response response = getResponse(servletResponse); - HttpFields fields; - if (getResponseHeadersMethod != null) { - fields = (HttpFields) ReflectionUtils.invokeMethod(getResponseHeadersMethod, response); - } - else { - fields = response.getHttpFields(); - } - return new HttpHeaders(new JettyHeadersAdapter(fields)); + return new HttpHeaders(new JettyHeadersAdapter(response.getHttpFields())); } private static Response getResponse(HttpServletResponse response) { @@ -169,53 +159,44 @@ public class JettyHttpHandlerAdapter extends ServletHttpHandlerAdapter { @Override protected int writeToOutputStream(DataBuffer dataBuffer) throws IOException { OutputStream output = getOutputStream(); - if (jetty11Present) { - if (output instanceof HttpOutput httpOutput) { - ByteBuffer input = dataBuffer.toByteBuffer(); - int len = input.remaining(); - httpOutput.write(input); - return len; - } + if (output instanceof HttpOutput httpOutput) { + ByteBuffer input = dataBuffer.toByteBuffer(); + int len = input.remaining(); + httpOutput.write(input); + return len; } - /* Jetty 12: see spring-web.gradle - else if (jetty12Present) { - if (output instanceof org.eclipse.jetty.ee10.servlet.HttpOutput httpOutput) { - ByteBuffer input = dataBuffer.toByteBuffer(); - int len = input.remaining(); - httpOutput.write(input); - return len; - } - } - */ return super.writeToOutputStream(dataBuffer); } @Override protected void applyHeaders() { - HttpServletResponse response = getNativeResponse(); - - MediaType contentType = null; - try { - contentType = getHeaders().getContentType(); - } - catch (Exception ex) { - String rawContentType = getHeaders().getFirst(HttpHeaders.CONTENT_TYPE); - response.setContentType(rawContentType); - } - if (response.getContentType() == null && contentType != null) { - response.setContentType(contentType.toString()); - } - - Charset charset = (contentType != null ? contentType.getCharset() : null); - if (response.getCharacterEncoding() == null && charset != null) { - response.setCharacterEncoding(charset.name()); - } - - long contentLength = getHeaders().getContentLength(); - if (contentLength != -1) { - response.setContentLengthLong(contentLength); - } + adaptHeaders(false); } } + + /* Jetty 12: see spring-web.gradle + private static final class Jetty12ServerHttpResponse extends ServletServerHttpResponse { + + Jetty12ServerHttpResponse(HttpServletResponse response, AsyncContext asyncContext, + DataBufferFactory bufferFactory, int bufferSize, ServletServerHttpRequest request) + throws IOException { + + super(response, asyncContext, bufferFactory, bufferSize, request); + } + + @Override + protected int writeToOutputStream(DataBuffer dataBuffer) throws IOException { + OutputStream output = getOutputStream(); + if (output instanceof org.eclipse.jetty.ee10.servlet.HttpOutput httpOutput) { + ByteBuffer input = dataBuffer.toByteBuffer(); + int len = input.remaining(); + httpOutput.write(input); + return len; + } + return super.writeToOutputStream(dataBuffer); + } + } + */ + } diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletHttpHandlerAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletHttpHandlerAdapter.java index d462f7b2e9..456f85e4b2 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletHttpHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletHttpHandlerAdapter.java @@ -169,7 +169,7 @@ public class ServletHttpHandlerAdapter implements Servlet { AsyncListener requestListener; String logPrefix; try { - httpRequest = createRequest(((HttpServletRequest) request), asyncContext); + httpRequest = createRequest((HttpServletRequest) request, asyncContext); requestListener = httpRequest.getAsyncListener(); logPrefix = httpRequest.getLogPrefix(); } @@ -182,8 +182,10 @@ public class ServletHttpHandlerAdapter implements Servlet { return; } - ServerHttpResponse httpResponse = createResponse(((HttpServletResponse) response), asyncContext, httpRequest); - AsyncListener responseListener = ((ServletServerHttpResponse) httpResponse).getAsyncListener(); + ServletServerHttpResponse wrappedResponse = + createResponse((HttpServletResponse) response, asyncContext, httpRequest); + ServerHttpResponse httpResponse = wrappedResponse; + AsyncListener responseListener = wrappedResponse.getAsyncListener(); if (httpRequest.getMethod() == HttpMethod.HEAD) { httpResponse = new HttpHeadResponseDecorator(httpResponse); } @@ -263,9 +265,7 @@ public class ServletHttpHandlerAdapter implements Servlet { private final String logPrefix; - - public HttpHandlerAsyncListener( - AsyncListener requestAsyncListener, AsyncListener responseAsyncListener, + public HttpHandlerAsyncListener(AsyncListener requestAsyncListener, AsyncListener responseAsyncListener, Runnable handlerDisposeTask, AtomicBoolean completionFlag, String logPrefix) { this.requestAsyncListener = requestAsyncListener; @@ -275,7 +275,6 @@ public class ServletHttpHandlerAdapter implements Servlet { this.logPrefix = logPrefix; } - @Override public void onTimeout(AsyncEvent event) { // Should never happen since we call asyncContext.setTimeout(-1) @@ -361,9 +360,7 @@ public class ServletHttpHandlerAdapter implements Servlet { @Nullable private volatile Subscription subscription; - public HandlerResultSubscriber( - AsyncContext asyncContext, AtomicBoolean completionFlag, String logPrefix) { - + public HandlerResultSubscriber(AsyncContext asyncContext, AtomicBoolean completionFlag, String logPrefix) { this.asyncContext = asyncContext; this.completionFlag = completionFlag; this.logPrefix = logPrefix; diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java index 032e2b9efd..0e34c6b725 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpResponse.java @@ -129,6 +129,11 @@ class ServletServerHttpResponse extends AbstractListenerServerHttpResponse { this.response.addHeader(headerName, headerValue); } }); + + adaptHeaders(false); + } + + protected void adaptHeaders(boolean removeAdaptedHeaders) { MediaType contentType = null; try { contentType = getHeaders().getContentType(); @@ -140,19 +145,25 @@ class ServletServerHttpResponse extends AbstractListenerServerHttpResponse { if (this.response.getContentType() == null && contentType != null) { this.response.setContentType(contentType.toString()); } + Charset charset = (contentType != null ? contentType.getCharset() : null); if (this.response.getCharacterEncoding() == null && charset != null) { this.response.setCharacterEncoding(charset.name()); } + long contentLength = getHeaders().getContentLength(); if (contentLength != -1) { this.response.setContentLengthLong(contentLength); } + + if (removeAdaptedHeaders) { + getHeaders().remove(HttpHeaders.CONTENT_TYPE); + getHeaders().remove(HttpHeaders.CONTENT_LENGTH); + } } @Override protected void applyCookies() { - // Servlet Cookie doesn't support same site: // https://github.com/eclipse-ee4j/servlet-api/issues/175 diff --git a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java index bbc9528b56..f302eb5f44 100644 --- a/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java +++ b/spring-web/src/main/java/org/springframework/http/server/reactive/TomcatHttpHandlerAdapter.java @@ -20,7 +20,6 @@ import java.io.IOException; import java.lang.reflect.Field; import java.net.URISyntaxException; import java.nio.ByteBuffer; -import java.nio.charset.Charset; import jakarta.servlet.AsyncContext; import jakarta.servlet.http.HttpServletRequest; @@ -37,7 +36,6 @@ import org.apache.coyote.Response; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferFactory; import org.springframework.http.HttpHeaders; -import org.springframework.http.MediaType; import org.springframework.util.Assert; import org.springframework.util.MultiValueMap; import org.springframework.util.ReflectionUtils; @@ -49,6 +47,7 @@ import org.springframework.util.ReflectionUtils; * @author Violeta Georgieva * @author Brian Clozel * @author Sam Brannen + * @author Juergen Hoeller * @since 5.0 * @see org.springframework.web.server.adapter.AbstractReactiveWebInitializer */ @@ -193,31 +192,7 @@ public class TomcatHttpHandlerAdapter extends ServletHttpHandlerAdapter { @Override protected void applyHeaders() { - HttpServletResponse response = getNativeResponse(); - - MediaType contentType = null; - try { - contentType = getHeaders().getContentType(); - } - catch (Exception ex) { - String rawContentType = getHeaders().getFirst(HttpHeaders.CONTENT_TYPE); - response.setContentType(rawContentType); - } - if (response.getContentType() == null && contentType != null) { - response.setContentType(contentType.toString()); - } - getHeaders().remove(HttpHeaders.CONTENT_TYPE); - - Charset charset = (contentType != null ? contentType.getCharset() : null); - if (response.getCharacterEncoding() == null && charset != null) { - response.setCharacterEncoding(charset.name()); - } - - long contentLength = getHeaders().getContentLength(); - if (contentLength != -1) { - response.setContentLengthLong(contentLength); - } - getHeaders().remove(HttpHeaders.CONTENT_LENGTH); + adaptHeaders(true); } @Override