From df99889aa6f45988a5879356d89e644f926c85af Mon Sep 17 00:00:00 2001 From: Rossen Stoyanchev Date: Fri, 8 May 2020 14:07:30 +0100 Subject: [PATCH] Avoided repeated creation of ReadOnlyHttpHeaders wrapper See gh-24680 --- .../org/springframework/http/HttpHeaders.java | 4 ++- .../client/AbstractClientHttpRequest.java | 17 ++++++++++-- .../reactive/AbstractClientHttpRequest.java | 17 +++++++++--- .../server/ServletServerHttpResponse.java | 16 +++++++++-- .../reactive/AbstractServerHttpResponse.java | 15 +++++++++-- .../client/DefaultClientResponse.java | 13 +++++---- .../client/DefaultWebClientBuilder.java | 8 ++---- .../function/server/DefaultServerRequest.java | 27 +++++++++---------- .../client/DefaultClientResponseTests.java | 18 +++++-------- .../function/DefaultServerRequest.java | 26 +++++++++--------- 10 files changed, 99 insertions(+), 62 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java index ff1e9b0cbd..8f2e0ba14c 100644 --- a/spring-web/src/main/java/org/springframework/http/HttpHeaders.java +++ b/spring-web/src/main/java/org/springframework/http/HttpHeaders.java @@ -1769,7 +1769,9 @@ public class HttpHeaders implements MultiValueMap, Serializable /** - * Apply a read-only {@code HttpHeaders} wrapper around the given headers. + * Apply a read-only {@code HttpHeaders} wrapper around the given headers + * that also caches the parsed representations of the "Accept" and + * "Content-Type" headers. */ public static HttpHeaders readOnlyHttpHeaders(MultiValueMap headers) { Assert.notNull(headers, "HttpHeaders must not be null"); diff --git a/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java index 2848d75f52..f964e66e80 100644 --- a/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/AbstractClientHttpRequest.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2014 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -20,6 +20,7 @@ import java.io.IOException; import java.io.OutputStream; import org.springframework.http.HttpHeaders; +import org.springframework.lang.Nullable; import org.springframework.util.Assert; /** @@ -35,10 +36,22 @@ public abstract class AbstractClientHttpRequest implements ClientHttpRequest { private boolean executed = false; + @Nullable + private HttpHeaders readOnlyHeaders; + @Override public final HttpHeaders getHeaders() { - return (this.executed ? HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (this.executed) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; + } } @Override diff --git a/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java b/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java index 5414dd7a62..8734521f04 100644 --- a/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java +++ b/spring-web/src/main/java/org/springframework/http/client/reactive/AbstractClientHttpRequest.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -60,6 +60,9 @@ public abstract class AbstractClientHttpRequest implements ClientHttpRequest { private final List>> commitActions = new ArrayList<>(4); + @Nullable + private HttpHeaders readOnlyHeaders; + public AbstractClientHttpRequest() { this(new HttpHeaders()); @@ -74,10 +77,16 @@ public abstract class AbstractClientHttpRequest implements ClientHttpRequest { @Override public HttpHeaders getHeaders() { - if (State.COMMITTED.equals(this.state.get())) { - return HttpHeaders.readOnlyHttpHeaders(this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (State.COMMITTED.equals(this.state.get())) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; } - return this.headers; } @Override diff --git a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java index d718971e68..45758ec0d9 100644 --- a/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java +++ b/spring-web/src/main/java/org/springframework/http/server/ServletServerHttpResponse.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -47,6 +47,9 @@ public class ServletServerHttpResponse implements ServerHttpResponse { private boolean bodyUsed = false; + @Nullable + private HttpHeaders readOnlyHeaders; + /** * Construct a new instance of the ServletServerHttpResponse based on the given {@link HttpServletResponse}. @@ -74,7 +77,16 @@ public class ServletServerHttpResponse implements ServerHttpResponse { @Override public HttpHeaders getHeaders() { - return (this.headersWritten ? HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (this.headersWritten) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; + } } @Override 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 b6c7f98037..517a2436ba 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 @@ -75,6 +75,9 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { private final List>> commitActions = new ArrayList<>(4); + @Nullable + private HttpHeaders readOnlyHeaders; + public AbstractServerHttpResponse(DataBufferFactory dataBufferFactory) { this(dataBufferFactory, new HttpHeaders()); @@ -155,8 +158,16 @@ public abstract class AbstractServerHttpResponse implements ServerHttpResponse { @Override public HttpHeaders getHeaders() { - return (this.state.get() == State.COMMITTED ? - HttpHeaders.readOnlyHttpHeaders(this.headers) : this.headers); + if (this.readOnlyHeaders != null) { + return this.readOnlyHeaders; + } + else if (this.state.get() == State.COMMITTED) { + this.readOnlyHeaders = HttpHeaders.readOnlyHttpHeaders(this.headers); + return this.readOnlyHeaders; + } + else { + return this.headers; + } } @Override diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java index f818e47568..53d6c7fc3c 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultClientResponse.java @@ -234,29 +234,28 @@ class DefaultClientResponse implements ClientResponse { private class DefaultHeaders implements Headers { - private HttpHeaders delegate() { - return response.getHeaders(); - } + private final HttpHeaders httpHeaders = + HttpHeaders.readOnlyHttpHeaders(response.getHeaders()); @Override public OptionalLong contentLength() { - return toOptionalLong(delegate().getContentLength()); + return toOptionalLong(this.httpHeaders.getContentLength()); } @Override public Optional contentType() { - return Optional.ofNullable(delegate().getContentType()); + return Optional.ofNullable(this.httpHeaders.getContentType()); } @Override public List header(String headerName) { - List headerValues = delegate().get(headerName); + List headerValues = this.httpHeaders.get(headerName); return (headerValues != null ? headerValues : Collections.emptyList()); } @Override public HttpHeaders asHttpHeaders() { - return HttpHeaders.readOnlyHttpHeaders(delegate()); + return this.httpHeaders; } private OptionalLong toOptionalLong(long value) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java index fed9093abd..0fa80f70dc 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/client/DefaultWebClientBuilder.java @@ -265,8 +265,8 @@ final class DefaultWebClientBuilder implements WebClient.Builder { .map(filter -> filter.apply(exchange)) .orElse(exchange) : exchange); return new DefaultWebClient(filteredExchange, initUriBuilderFactory(), - this.defaultHeaders != null ? unmodifiableCopy(this.defaultHeaders) : null, - this.defaultCookies != null ? unmodifiableCopy(this.defaultCookies) : null, + this.defaultHeaders != null ? HttpHeaders.readOnlyHttpHeaders(this.defaultHeaders) : null, + this.defaultCookies != null ? HttpHeaders.readOnlyHttpHeaders(this.defaultCookies) : null, this.defaultRequest, new DefaultWebClientBuilder(this)); } @@ -308,10 +308,6 @@ final class DefaultWebClientBuilder implements WebClient.Builder { return factory; } - private static HttpHeaders unmodifiableCopy(HttpHeaders headers) { - return HttpHeaders.readOnlyHttpHeaders(headers); - } - private static MultiValueMap unmodifiableCopy(MultiValueMap map) { return CollectionUtils.unmodifiableMultiValueMap(new LinkedMultiValueMap<>(map)); } diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java index e1b12d8858..8ea642a90d 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/function/server/DefaultServerRequest.java @@ -260,61 +260,60 @@ class DefaultServerRequest implements ServerRequest { private class DefaultHeaders implements Headers { - - private HttpHeaders delegate() { - return request().getHeaders(); - } + + private final HttpHeaders httpHeaders = + HttpHeaders.readOnlyHttpHeaders(request().getHeaders()); @Override public List accept() { - return delegate().getAccept(); + return this.httpHeaders.getAccept(); } @Override public List acceptCharset() { - return delegate().getAcceptCharset(); + return this.httpHeaders.getAcceptCharset(); } @Override public List acceptLanguage() { - return delegate().getAcceptLanguage(); + return this.httpHeaders.getAcceptLanguage(); } @Override public OptionalLong contentLength() { - long value = delegate().getContentLength(); + long value = this.httpHeaders.getContentLength(); return (value != -1 ? OptionalLong.of(value) : OptionalLong.empty()); } @Override public Optional contentType() { - return Optional.ofNullable(delegate().getContentType()); + return Optional.ofNullable(this.httpHeaders.getContentType()); } @Override public InetSocketAddress host() { - return delegate().getHost(); + return this.httpHeaders.getHost(); } @Override public List range() { - return delegate().getRange(); + return this.httpHeaders.getRange(); } @Override public List header(String headerName) { - List headerValues = delegate().get(headerName); + List headerValues = this.httpHeaders.get(headerName); return (headerValues != null ? headerValues : Collections.emptyList()); } @Override public HttpHeaders asHttpHeaders() { - return HttpHeaders.readOnlyHttpHeaders(delegate()); + return this.httpHeaders; } @Override public String toString() { - return delegate().toString(); + return this.httpHeaders.toString(); } } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java index ed05d7b2d7..5fe2940400 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/function/client/DefaultClientResponseTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2019 the original author or authors. + * Copyright 2002-2020 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -60,6 +60,8 @@ public class DefaultClientResponseTests { private ClientHttpResponse mockResponse; + private final HttpHeaders httpHeaders = new HttpHeaders(); + private ExchangeStrategies mockExchangeStrategies; private DefaultClientResponse defaultClientResponse; @@ -68,6 +70,7 @@ public class DefaultClientResponseTests { @BeforeEach public void createMocks() { mockResponse = mock(ClientHttpResponse.class); + given(mockResponse.getHeaders()).willReturn(this.httpHeaders); mockExchangeStrategies = mock(ExchangeStrategies.class); defaultClientResponse = new DefaultClientResponse(mockResponse, mockExchangeStrategies, "", "", () -> null); } @@ -91,7 +94,6 @@ public class DefaultClientResponseTests { @Test public void header() { - HttpHeaders httpHeaders = new HttpHeaders(); long contentLength = 42L; httpHeaders.setContentLength(contentLength); MediaType contentType = MediaType.TEXT_PLAIN; @@ -233,7 +235,6 @@ public class DefaultClientResponseTests { = factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); Flux body = Flux.just(dataBuffer); - HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.setContentType(MediaType.TEXT_PLAIN); given(mockResponse.getHeaders()).willReturn(httpHeaders); given(mockResponse.getStatusCode()).willThrow(new IllegalArgumentException("999")); @@ -293,13 +294,12 @@ public class DefaultClientResponseTests { } @Test - public void toEntityListWithUnknownStatusCode() throws Exception { + public void toEntityListWithUnknownStatusCode() { DefaultDataBufferFactory factory = new DefaultDataBufferFactory(); DefaultDataBuffer dataBuffer = factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); Flux body = Flux.just(dataBuffer); - HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.setContentType(MediaType.TEXT_PLAIN); given(mockResponse.getHeaders()).willReturn(httpHeaders); given(mockResponse.getStatusCode()).willThrow(new IllegalArgumentException("999")); @@ -321,8 +321,7 @@ public class DefaultClientResponseTests { @Test public void toEntityListTypeReference() { DefaultDataBufferFactory factory = new DefaultDataBufferFactory(); - DefaultDataBuffer dataBuffer = - factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); + DefaultDataBuffer dataBuffer = factory.wrap(ByteBuffer.wrap("foo".getBytes(StandardCharsets.UTF_8))); Flux body = Flux.just(dataBuffer); mockTextPlainResponse(body); @@ -332,8 +331,7 @@ public class DefaultClientResponseTests { given(mockExchangeStrategies.messageReaders()).willReturn(messageReaders); ResponseEntity> result = defaultClientResponse.toEntityList( - new ParameterizedTypeReference() { - }).block(); + new ParameterizedTypeReference() {}).block(); assertThat(result.getBody()).isEqualTo(Collections.singletonList("foo")); assertThat(result.getStatusCode()).isEqualTo(HttpStatus.OK); assertThat(result.getStatusCodeValue()).isEqualTo(HttpStatus.OK.value()); @@ -342,9 +340,7 @@ public class DefaultClientResponseTests { private void mockTextPlainResponse(Flux body) { - HttpHeaders httpHeaders = new HttpHeaders(); httpHeaders.setContentType(MediaType.TEXT_PLAIN); - given(mockResponse.getHeaders()).willReturn(httpHeaders); given(mockResponse.getStatusCode()).willReturn(HttpStatus.OK); given(mockResponse.getRawStatusCode()).willReturn(HttpStatus.OK.value()); given(mockResponse.getBody()).willReturn(body); diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java index a3f5fe8f4a..d5405ef9e8 100644 --- a/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java +++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/function/DefaultServerRequest.java @@ -305,62 +305,62 @@ class DefaultServerRequest implements ServerRequest { */ static class DefaultRequestHeaders implements Headers { - private final HttpHeaders delegate; + private final HttpHeaders httpHeaders; - public DefaultRequestHeaders(HttpHeaders delegate) { - this.delegate = delegate; + public DefaultRequestHeaders(HttpHeaders httpHeaders) { + this.httpHeaders = HttpHeaders.readOnlyHttpHeaders(httpHeaders); } @Override public List accept() { - return this.delegate.getAccept(); + return this.httpHeaders.getAccept(); } @Override public List acceptCharset() { - return this.delegate.getAcceptCharset(); + return this.httpHeaders.getAcceptCharset(); } @Override public List acceptLanguage() { - return this.delegate.getAcceptLanguage(); + return this.httpHeaders.getAcceptLanguage(); } @Override public OptionalLong contentLength() { - long value = this.delegate.getContentLength(); + long value = this.httpHeaders.getContentLength(); return (value != -1 ? OptionalLong.of(value) : OptionalLong.empty()); } @Override public Optional contentType() { - return Optional.ofNullable(this.delegate.getContentType()); + return Optional.ofNullable(this.httpHeaders.getContentType()); } @Override public InetSocketAddress host() { - return this.delegate.getHost(); + return this.httpHeaders.getHost(); } @Override public List range() { - return this.delegate.getRange(); + return this.httpHeaders.getRange(); } @Override public List header(String headerName) { - List headerValues = this.delegate.get(headerName); + List headerValues = this.httpHeaders.get(headerName); return (headerValues != null ? headerValues : Collections.emptyList()); } @Override public HttpHeaders asHttpHeaders() { - return HttpHeaders.readOnlyHttpHeaders(this.delegate); + return this.httpHeaders; } @Override public String toString() { - return this.delegate.toString(); + return this.httpHeaders.toString(); } }