Thread-safe reactive ServerHttpRequest

This commit replaces the lazy URI and headers initialization in the
reactive ServerHttpRequest in favor of eager initialization at
construction time. Both the URI and headers are nearly guaranteed to
be accessed for every request (URI for application path, headers for
"Origin" header).

Query params are still lazily parsed but parsing is idemptotent and
in the unlikely case of concurrent access (it's the framework that
typically accesses query params) it maybe parsed twice but should
be side effect free still.

Cookies are also parsed lazily and since we delegate to the "native"
request, it depends on the underlying runtime whether synchronization
is needed. This commit adds synchronization for the HttpServletRequest.
At present RxNetty, Reactor, and Undertow implementations provide
thread-safe access to cookies.

spring-web/src/main/java/org/springframework/http/server/reactive/AbstractServerHttpRequest.java

@@ -17,7 +17,6 @@
 package org.springframework.http.server.reactive;
 
 import java.net.URI;
-import java.net.URISyntaxException;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
@@ -39,28 +38,36 @@ public abstract class AbstractServerHttpRequest implements ServerHttpRequest {
 	private static final Pattern QUERY_PATTERN = Pattern.compile("([^&=]+)(=?)([^&]+)?");
 
 
-	private URI uri;
+	private final URI uri;
 
-	private MultiValueMap<String, String> queryParams;
+	private final HttpHeaders headers;
 
-	private HttpHeaders headers;
+	private MultiValueMap<String, String> queryParams;
 
 	private MultiValueMap<String, HttpCookie> cookies;
 
 
+	/**
+	 * Constructor with the URI and headers for the request.
+	 * @param uri the URI for the request
+	 * @param headers the headers for the request
+	 */
+	public AbstractServerHttpRequest(URI uri, HttpHeaders headers) {
+		this.uri = uri;
+		this.headers = HttpHeaders.readOnlyHttpHeaders(headers);
+	}
+
+
 	@Override
 	public URI getURI() {
-		if (this.uri == null) {
-			try {
-				this.uri = initUri();
-			}
-			catch (URISyntaxException ex) {
-				throw new IllegalStateException("Could not get URI: " + ex.getMessage(), ex);
-			}
-		}
 		return this.uri;
 	}
 
+	@Override
+	public HttpHeaders getHeaders() {
+		return this.headers;
+	}
+
 	@Override
 	public MultiValueMap<String, String> getQueryParams() {
 		if (this.queryParams == null) {
@@ -69,6 +76,14 @@ public abstract class AbstractServerHttpRequest implements ServerHttpRequest {
 		return this.queryParams;
 	}
 
+	/**
+	 * A method for parsing of the query into name-value pairs. The return
+	 * value is turned into an immutable map and cached.
+	 *
+	 * <p>Note that this method is invoked lazily on first access to
+	 * {@link #getQueryParams()}. The invocation is not synchronized but the
+	 * parsing is thread-safe nevertheless.
+	 */
 	protected MultiValueMap<String, String> initQueryParams() {
 		MultiValueMap<String, String> queryParams = new LinkedMultiValueMap<>();
 		String query = getURI().getRawQuery();
@@ -85,14 +100,6 @@ public abstract class AbstractServerHttpRequest implements ServerHttpRequest {
 		return queryParams;
 	}
 
-	@Override
-	public HttpHeaders getHeaders() {
-		if (this.headers == null) {
-			this.headers = HttpHeaders.readOnlyHttpHeaders(initHeaders());
-		}
-		return this.headers;
-	}
-
 	@Override
 	public MultiValueMap<String, HttpCookie> getCookies() {
 		if (this.cookies == null) {
@@ -101,24 +108,14 @@ public abstract class AbstractServerHttpRequest implements ServerHttpRequest {
 		return this.cookies;
 	}
 
-
-	/**
-	 * Initialize a URI that represents the request.
-	 * <p>Invoked lazily on the first call to {@link #getURI()} and then cached.
-	 * @throws URISyntaxException
-	 */
-	protected abstract URI initUri() throws URISyntaxException;
-
-	/**
-	 * Initialize the headers from the underlying request.
-	 * <p>Invoked lazily on the first call to {@link #getHeaders()} and then cached.
-	 */
-	protected abstract HttpHeaders initHeaders();
-
 	/**
-	 * Initialize the cookies from the underlying request.
-	 * <p>Invoked lazily on the first access to cookies via {@link #getHeaders()}
-	 * and then cached.
+	 * Obtain the cookies from the underlying "native" request and adapt those to
+	 * an {@link HttpCookie} map. The return value is turned into an immutable
+	 * map and cached.
+	 * <p>Note that this method is invoked lazily on access to
+	 * {@link #getCookies()}. Sub-classes should synchronize cookie
+	 * initialization if the underlying "native" request does not provide
+	 * thread-safe access to cookie data.
 	 */
 	protected abstract MultiValueMap<String, HttpCookie> initCookies();
 

spring-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java

@@ -37,6 +37,7 @@ import org.springframework.util.MultiValueMap;
  * Adapt {@link ServerHttpRequest} to the Reactor Net {@link HttpChannel}.
  *
  * @author Stephane Maldini
+ * @author Rossen Stoyanchev
  * @since 5.0
  */
 public class ReactorServerHttpRequest extends AbstractServerHttpRequest {
@@ -46,13 +47,40 @@ public class ReactorServerHttpRequest extends AbstractServerHttpRequest {
 	private final NettyDataBufferFactory bufferFactory;
 
 
-	public ReactorServerHttpRequest(HttpChannel request, NettyDataBufferFactory bufferFactory) {
-		Assert.notNull("'request' must not be null");
+	public ReactorServerHttpRequest(HttpChannel channel, NettyDataBufferFactory bufferFactory) {
+		super(initUri(channel), initHeaders(channel));
 		Assert.notNull(bufferFactory, "'bufferFactory' must not be null");
-		this.channel = request;
+		this.channel = channel;
 		this.bufferFactory = bufferFactory;
 	}
 
+	private static URI initUri(HttpChannel channel) {
+		Assert.notNull("'channel' must not be null");
+		try {
+			URI uri = new URI(channel.uri());
+			InetSocketAddress remoteAddress = channel.remoteAddress();
+			return new URI(
+					uri.getScheme(),
+					uri.getUserInfo(),
+					(remoteAddress != null ? remoteAddress.getHostString() : null),
+					(remoteAddress != null ? remoteAddress.getPort() : -1),
+					uri.getPath(),
+					uri.getQuery(),
+					uri.getFragment());
+		}
+		catch (URISyntaxException ex) {
+			throw new IllegalStateException("Could not get URI: " + ex.getMessage(), ex);
+		}
+	}
+
+	private static HttpHeaders initHeaders(HttpChannel channel) {
+		HttpHeaders headers = new HttpHeaders();
+		for (String name : channel.headers().names()) {
+			headers.put(name, channel.headers().getAll(name));
+		}
+		return headers;
+	}
+
 
 	public HttpChannel getReactorChannel() {
 		return this.channel;
@@ -63,29 +91,6 @@ public class ReactorServerHttpRequest extends AbstractServerHttpRequest {
 		return HttpMethod.valueOf(this.channel.method().name());
 	}
 
-	@Override
-	protected URI initUri() throws URISyntaxException {
-		URI uri = new URI(this.channel.uri());
-		InetSocketAddress remoteAddress = this.channel.remoteAddress();
-		return new URI(
-				uri.getScheme(),
-				uri.getUserInfo(),
-				(remoteAddress != null ? remoteAddress.getHostString() : null),
-				(remoteAddress != null ? remoteAddress.getPort() : -1),
-				uri.getPath(),
-				uri.getQuery(),
-				uri.getFragment());
-	}
-
-	@Override
-	protected HttpHeaders initHeaders() {
-		HttpHeaders headers = new HttpHeaders();
-		for (String name : this.channel.headers().names()) {
-			headers.put(name, this.channel.headers().getAll(name));
-		}
-		return headers;
-	}
-
 	@Override
 	protected MultiValueMap<String, HttpCookie> initCookies() {
 		MultiValueMap<String, HttpCookie> cookies = new LinkedMultiValueMap<>();

spring-web/src/main/java/org/springframework/http/server/reactive/RxNettyServerHttpRequest.java

@@ -19,6 +19,7 @@ package org.springframework.http.server.reactive;
 import java.net.InetSocketAddress;
 import java.net.URI;
 import java.net.URISyntaxException;
+import java.util.HashMap;
 
 import io.netty.buffer.ByteBuf;
 import io.netty.handler.codec.http.cookie.Cookie;
@@ -35,6 +36,7 @@ import org.springframework.http.HttpMethod;
 import org.springframework.util.Assert;
 import org.springframework.util.LinkedMultiValueMap;
 import org.springframework.util.MultiValueMap;
+import org.springframework.util.StringUtils;
 
 /**
  * Adapt {@link ServerHttpRequest} to the RxNetty {@link HttpServerRequest}.
@@ -49,46 +51,57 @@ public class RxNettyServerHttpRequest extends AbstractServerHttpRequest {
 
 	private final NettyDataBufferFactory dataBufferFactory;
 
+
 	public RxNettyServerHttpRequest(HttpServerRequest<ByteBuf> request,
 			NettyDataBufferFactory dataBufferFactory) {
-		Assert.notNull("'request', request must not be null");
+
+		super(initUri(request), initHeaders(request));
+
 		Assert.notNull(dataBufferFactory, "'dataBufferFactory' must not be null");
-		this.dataBufferFactory = dataBufferFactory;
 		this.request = request;
+		this.dataBufferFactory = dataBufferFactory;
 	}
 
-
-	public HttpServerRequest<ByteBuf> getRxNettyRequest() {
-		return this.request;
+	private static URI initUri(HttpServerRequest<ByteBuf> request) {
+		Assert.notNull("'request', request must not be null");
+		try {
+			URI uri = new URI(request.getUri());
+			InetSocketAddress remoteAddress = null;
+			if (!StringUtils.isEmpty(request.getHostHeader())) {
+				HttpHeaders headers = new HttpHeaders();
+				headers.add("Host", request.getHostHeader());
+				remoteAddress = headers.getHost();
+			}
+			return new URI(
+					uri.getScheme(),
+					uri.getUserInfo(),
+					(remoteAddress != null ? remoteAddress.getHostString() : null),
+					(remoteAddress != null ? remoteAddress.getPort() : -1),
+					uri.getPath(),
+					uri.getQuery(),
+					uri.getFragment());
+		}
+		catch (URISyntaxException ex) {
+			throw new IllegalStateException("Could not get URI: " + ex.getMessage(), ex);
+		}
 	}
 
-	@Override
-	public HttpMethod getMethod() {
-		return HttpMethod.valueOf(this.request.getHttpMethod().name());
+	private static HttpHeaders initHeaders(HttpServerRequest<ByteBuf> request) {
+		HttpHeaders headers = new HttpHeaders();
+		for (String name : request.getHeaderNames()) {
+			headers.put(name, request.getAllHeaderValues(name));
+		}
+		return headers;
 	}
 
-	@Override
-	protected URI initUri() throws URISyntaxException {
-		URI uri = new URI(this.request.getUri());
-		InetSocketAddress remoteAddress = this.getHeaders().getHost();
-		return new URI(
-				uri.getScheme(),
-				uri.getUserInfo(),
-				(remoteAddress != null ? remoteAddress.getHostString() : null),
-				(remoteAddress != null ? remoteAddress.getPort() : -1),
-				uri.getPath(),
-				uri.getQuery(),
-				uri.getFragment());
 
+	public HttpServerRequest<ByteBuf> getRxNettyRequest() {
+		return this.request;
 	}
 
 	@Override
-	protected HttpHeaders initHeaders() {
-		HttpHeaders headers = new HttpHeaders();
-		for (String name : this.request.getHeaderNames()) {
-			headers.put(name, this.request.getAllHeaderValues(name));
-		}
-		return headers;
+	public HttpMethod getMethod() {
+		return HttpMethod.valueOf(this.request.getHttpMethod().name());
 	}
 
 	@Override

spring-web/src/main/java/org/springframework/http/server/reactive/ServletServerHttpRequest.java

@@ -59,64 +59,56 @@ public class ServletServerHttpRequest extends AbstractServerHttpRequest {
 
 	private volatile RequestBodyPublisher bodyPublisher;
 
+	private final Object cookieLock = new Object();
+
 
 	public ServletServerHttpRequest(HttpServletRequest request,
-			DataBufferFactory dataBufferFactory, int bufferSize) {
+			DataBufferFactory bufferFactory, int bufferSize) {
+
+		super(initUri(request), initHeaders(request));
 
-		Assert.notNull(request, "HttpServletRequest must not be null");
-		Assert.notNull(dataBufferFactory, "DataBufferFactory must not be null");
-		Assert.isTrue(bufferSize > 0, "Buffer size must be higher than 0");
+		Assert.notNull(bufferFactory, "'bufferFactory' must not be null");
+		Assert.isTrue(bufferSize > 0, "'bufferSize' must be higher than 0");
 		this.request = request;
-		this.dataBufferFactory = dataBufferFactory;
+		this.dataBufferFactory = bufferFactory;
 		this.bufferSize = bufferSize;
 	}
 
-
-	public HttpServletRequest getServletRequest() {
-		return this.request;
-	}
-
-	@Override
-	public HttpMethod getMethod() {
-		return HttpMethod.valueOf(getServletRequest().getMethod());
-	}
-
-	@Override
-	public String getContextPath() {
-		return getServletRequest().getContextPath();
-	}
-
-	@Override
-	protected URI initUri() throws URISyntaxException {
-		StringBuffer url = this.request.getRequestURL();
-		String query = this.request.getQueryString();
-		if (StringUtils.hasText(query)) {
-			url.append('?').append(query);
+	private static URI initUri(HttpServletRequest request) {
+		Assert.notNull(request, "'request' must not be null");
+		try {
+			StringBuffer url = request.getRequestURL();
+			String query = request.getQueryString();
+			if (StringUtils.hasText(query)) {
+				url.append('?').append(query);
+			}
+			return new URI(url.toString());
+		}
+		catch (URISyntaxException ex) {
+			throw new IllegalStateException("Could not get URI: " + ex.getMessage(), ex);
 		}
-		return new URI(url.toString());
 	}
 
-	@Override
-	protected HttpHeaders initHeaders() {
+	private static HttpHeaders initHeaders(HttpServletRequest request) {
 		HttpHeaders headers = new HttpHeaders();
-		for (Enumeration<?> names = getServletRequest().getHeaderNames();
-		     names.hasMoreElements(); ) {
+		for (Enumeration<?> names = request.getHeaderNames();
+			 names.hasMoreElements(); ) {
 			String name = (String) names.nextElement();
-			for (Enumeration<?> values = getServletRequest().getHeaders(name);
-			     values.hasMoreElements(); ) {
+			for (Enumeration<?> values = request.getHeaders(name);
+				 values.hasMoreElements(); ) {
 				headers.add(name, (String) values.nextElement());
 			}
 		}
 		MediaType contentType = headers.getContentType();
 		if (contentType == null) {
-			String requestContentType = getServletRequest().getContentType();
+			String requestContentType = request.getContentType();
 			if (StringUtils.hasLength(requestContentType)) {
 				contentType = MediaType.parseMediaType(requestContentType);
 				headers.setContentType(contentType);
 			}
 		}
 		if (contentType != null && contentType.getCharset() == null) {
-			String encoding = getServletRequest().getCharacterEncoding();
+			String encoding = request.getCharacterEncoding();
 			if (StringUtils.hasLength(encoding)) {
 				Charset charset = Charset.forName(encoding);
 				Map<String, String> params = new LinkedCaseInsensitiveMap<>();
@@ -128,7 +120,7 @@ public class ServletServerHttpRequest extends AbstractServerHttpRequest {
 			}
 		}
 		if (headers.getContentLength() == -1) {
-			int contentLength = getServletRequest().getContentLength();
+			int contentLength = request.getContentLength();
 			if (contentLength != -1) {
 				headers.setContentLength(contentLength);
 			}
@@ -136,10 +128,28 @@ public class ServletServerHttpRequest extends AbstractServerHttpRequest {
 		return headers;
 	}
 
+
+	public HttpServletRequest getServletRequest() {
+		return this.request;
+	}
+
+	@Override
+	public HttpMethod getMethod() {
+		return HttpMethod.valueOf(getServletRequest().getMethod());
+	}
+
+	@Override
+	public String getContextPath() {
+		return getServletRequest().getContextPath();
+	}
+
 	@Override
 	protected MultiValueMap<String, HttpCookie> initCookies() {
 		MultiValueMap<String, HttpCookie> httpCookies = new LinkedMultiValueMap<>();
-		Cookie[] cookies = this.request.getCookies();
+		Cookie[] cookies;
+		synchronized (this.cookieLock) {
+			cookies = this.request.getCookies();
+		}
 		if (cookies != null) {
 			for (Cookie cookie : cookies) {
 				String name = cookie.getName();

spring-web/src/main/java/org/springframework/http/server/reactive/UndertowServerHttpRequest.java

@@ -53,12 +53,33 @@ public class UndertowServerHttpRequest extends AbstractServerHttpRequest {
 
 	public UndertowServerHttpRequest(HttpServerExchange exchange,
 			DataBufferFactory dataBufferFactory) {
-		Assert.notNull(exchange, "'exchange' is required.");
+
+		super(initUri(exchange), initHeaders(exchange));
 		this.exchange = exchange;
 		this.body = new RequestBodyPublisher(exchange, dataBufferFactory);
 		this.body.registerListener();
 	}
 
+	private static URI initUri(HttpServerExchange exchange) {
+		Assert.notNull(exchange, "'exchange' is required.");
+		try {
+			return new URI(exchange.getRequestScheme(), null,
+					exchange.getHostName(), exchange.getHostPort(),
+					exchange.getRequestURI(), exchange.getQueryString(), null);
+		}
+		catch (URISyntaxException ex) {
+			throw new IllegalStateException("Could not get URI: " + ex.getMessage(), ex);
+		}
+	}
+
+	private static HttpHeaders initHeaders(HttpServerExchange exchange) {
+		HttpHeaders headers = new HttpHeaders();
+		for (HeaderValues values : exchange.getRequestHeaders()) {
+			headers.put(values.getHeaderName().toString(), values);
+		}
+		return headers;
+	}
+
 
 	public HttpServerExchange getUndertowExchange() {
 		return this.exchange;
@@ -69,22 +90,6 @@ public class UndertowServerHttpRequest extends AbstractServerHttpRequest {
 		return HttpMethod.valueOf(this.getUndertowExchange().getRequestMethod().toString());
 	}
 
-	@Override
-	protected URI initUri() throws URISyntaxException {
-		return new URI(this.exchange.getRequestScheme(), null,
-				this.exchange.getHostName(), this.exchange.getHostPort(),
-				this.exchange.getRequestURI(), this.exchange.getQueryString(), null);
-	}
-
-	@Override
-	protected HttpHeaders initHeaders() {
-		HttpHeaders headers = new HttpHeaders();
-		for (HeaderValues values : this.getUndertowExchange().getRequestHeaders()) {
-			headers.put(values.getHeaderName().toString(), values);
-		}
-		return headers;
-	}
-
 	@Override
 	protected MultiValueMap<String, HttpCookie> initCookies() {
 		MultiValueMap<String, HttpCookie> cookies = new LinkedMultiValueMap<>();