Skip to content

Optimize HTTP headers parsing #1977

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Oct 11, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 39 additions & 35 deletions spring-web/src/main/java/org/springframework/http/HttpHeaders.java
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@
import java.util.Collections;
import java.util.EnumSet;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedList;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand All @@ -47,7 +45,9 @@

import org.springframework.lang.Nullable;
import org.springframework.util.Assert;
import org.springframework.util.CollectionUtils;
import org.springframework.util.LinkedCaseInsensitiveMap;
import org.springframework.util.LinkedMultiValueMap;
import org.springframework.util.MultiValueMap;
import org.springframework.util.StringUtils;

Expand Down Expand Up @@ -78,7 +78,8 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
/**
* The empty {@code HttpHeaders} instance (immutable).
*/
public static final HttpHeaders EMPTY = new HttpHeaders(new LinkedHashMap<>(), true);
public static final HttpHeaders EMPTY =
new ReadOnlyHttpHeaders(new HttpHeaders(new LinkedMultiValueMap<>(0)));
/**
* The HTTP {@code Accept} header field name.
* @see <a href="http://tools.ietf.org/html/rfc7231#section-5.3.2">Section 5.3.2 of RFC 7231</a>
Expand Down Expand Up @@ -397,35 +398,27 @@ public class HttpHeaders implements MultiValueMap<String, String>, Serializable
private static final DateTimeFormatter[] DATE_FORMATTERS = new DateTimeFormatter[] {
DateTimeFormatter.RFC_1123_DATE_TIME,
DateTimeFormatter.ofPattern("EEEE, dd-MMM-yy HH:mm:ss zz", Locale.US),
DateTimeFormatter.ofPattern("EEE MMM dd HH:mm:ss yyyy",Locale.US).withZone(GMT)
DateTimeFormatter.ofPattern("EEE MMM dd HH:mm:ss yyyy", Locale.US).withZone(GMT)
};


private final Map<String, List<String>> headers;

private final boolean readOnly;
final MultiValueMap<String, String> headers;


/**
* Constructs a new, empty instance of the {@code HttpHeaders} object.
* Construct a new, empty instance of the {@code HttpHeaders} object.
*/
public HttpHeaders() {
this(new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH), false);
this(CollectionUtils.toMultiValueMap(
new LinkedCaseInsensitiveMap<>(8, Locale.ENGLISH)));
}

/**
* Private constructor that can create read-only {@code HttpHeader} instances.
* Construct a new {@code HttpHeaders} instance backed by an existing map.
*/
private HttpHeaders(Map<String, List<String>> headers, boolean readOnly) {
if (readOnly) {
Map<String, List<String>> map = new LinkedCaseInsensitiveMap<>(headers.size(), Locale.ENGLISH);
headers.forEach((key, valueList) -> map.put(key, Collections.unmodifiableList(valueList)));
this.headers = Collections.unmodifiableMap(map);
}
else {
this.headers = headers;
}
this.readOnly = readOnly;
public HttpHeaders(MultiValueMap<String, String> headers) {
Assert.notNull(headers, "headers must not be null");
this.headers = headers;
}


Expand Down Expand Up @@ -1474,8 +1467,7 @@ protected String toCommaDelimitedString(List<String> headerValues) {
@Override
@Nullable
public String getFirst(String headerName) {
List<String> headerValues = this.headers.get(headerName);
return (headerValues != null ? headerValues.get(0) : null);
return this.headers.getFirst(headerName);
}

/**
Expand All @@ -1488,19 +1480,17 @@ public String getFirst(String headerName) {
*/
@Override
public void add(String headerName, @Nullable String headerValue) {
List<String> headerValues = this.headers.computeIfAbsent(headerName, k -> new LinkedList<>());
headerValues.add(headerValue);
this.headers.add(headerName, headerValue);
}

@Override
public void addAll(String key, List<? extends String> values) {
List<String> currentValues = this.headers.computeIfAbsent(key, k -> new LinkedList<>());
currentValues.addAll(values);
this.headers.addAll(key, values);
}

@Override
public void addAll(MultiValueMap<String, String> values) {
values.forEach(this::addAll);
this.headers.addAll(values);
}

/**
Expand All @@ -1513,21 +1503,17 @@ public void addAll(MultiValueMap<String, String> values) {
*/
@Override
public void set(String headerName, @Nullable String headerValue) {
List<String> headerValues = new LinkedList<>();
headerValues.add(headerValue);
this.headers.put(headerName, headerValues);
this.headers.set(headerName, headerValue);
}

@Override
public void setAll(Map<String, String> values) {
values.forEach(this::set);
this.headers.setAll(values);
}

@Override
public Map<String, String> toSingleValueMap() {
LinkedHashMap<String, String> singleValueMap = new LinkedHashMap<>(this.headers.size());
this.headers.forEach((key, valueList) -> singleValueMap.put(key, valueList.get(0)));
return singleValueMap;
return this.headers.toSingleValueMap();
}


Expand Down Expand Up @@ -1623,7 +1609,25 @@ public String toString() {
*/
public static HttpHeaders readOnlyHttpHeaders(HttpHeaders headers) {
Assert.notNull(headers, "HttpHeaders must not be null");
return (headers.readOnly ? headers : new HttpHeaders(headers, true));
if (headers instanceof ReadOnlyHttpHeaders) {
return headers;
}
else {
return new ReadOnlyHttpHeaders(headers);
}
}

/**
* Return a {@code HttpHeaders} object that can read and written to.
*/
public static HttpHeaders writableHttpHeaders(HttpHeaders headers) {
Assert.notNull(headers, "HttpHeaders must not be null");
if (headers instanceof ReadOnlyHttpHeaders) {
return new HttpHeaders(headers.headers);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If ReadOnlyHttpHeaders is a wrapper around HttpHeaders, couldn't we check here if headers.headers is an instance of HttpHeaders and if so return it directly?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, headers.headers is itself a MultiValueMap. @poutsma and I changed ReadOnlyHttpHeaders so that it extends HttpHeaders (and calls super(MultiValueMap)) to avoid issues on the equals contract.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I see.

}
else {
return headers;
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,135 @@
/*
* Copyright 2002-2018 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.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.springframework.http;

import java.util.AbstractMap;
import java.util.Collection;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.stream.Collectors;

import org.springframework.lang.Nullable;
import org.springframework.util.MultiValueMap;

/**
* {@code HttpHeaders} object that can only be read, not written to.
*
* @author Brian Clozel
* @since 5.1
*/
class ReadOnlyHttpHeaders extends HttpHeaders {

private static final long serialVersionUID = -8578554704772377436L;

@Nullable
private MediaType cachedContentType;

ReadOnlyHttpHeaders(HttpHeaders headers) {
super(headers.headers);
}

@Override
public MediaType getContentType() {
if (this.cachedContentType != null) {
return this.cachedContentType;
}
else {
MediaType contentType = super.getContentType();
this.cachedContentType = contentType;
return contentType;
}
}

@Override
public List<String> get(Object key) {
List<String> values = this.headers.get(key);
if (values != null) {
return Collections.unmodifiableList(values);
}
return values;
}

@Override
public void add(String headerName, @Nullable String headerValue) {
throw new UnsupportedOperationException();
}

@Override
public void addAll(String key, List<? extends String> values) {
throw new UnsupportedOperationException();
}

@Override
public void addAll(MultiValueMap<String, String> values) {
throw new UnsupportedOperationException();
}

@Override
public void set(String headerName, @Nullable String headerValue) {
throw new UnsupportedOperationException();
}

@Override
public void setAll(Map<String, String> values) {
throw new UnsupportedOperationException();
}

@Override
public Map<String, String> toSingleValueMap() {
return Collections.unmodifiableMap(this.headers.toSingleValueMap());
}

@Override
public Set<String> keySet() {
return Collections.unmodifiableSet(this.headers.keySet());
}

@Override
public List<String> put(String key, List<String> value) {
throw new UnsupportedOperationException();
}

@Override
public List<String> remove(Object key) {
throw new UnsupportedOperationException();
}

@Override
public void putAll(Map<? extends String, ? extends List<String>> map) {
throw new UnsupportedOperationException();
}

@Override
public void clear() {
throw new UnsupportedOperationException();
}

@Override
public Collection<List<String>> values() {
return Collections.unmodifiableCollection(this.headers.values());
}

@Override
public Set<Entry<String, List<String>>> entrySet() {
return Collections.unmodifiableSet(this.headers.entrySet().stream()
.map(AbstractMap.SimpleImmutableEntry::new)
.collect(Collectors.toSet()));
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,9 @@ public List<String> get(Object key) {
Assert.isInstanceOf(String.class, key, "Key must be a String-based header name");

Collection<String> values1 = servletResponse.getHeaders((String) key);
if (headersWritten) {
return new ArrayList<>(values1);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without that change, I'm seeing duplicate response headers because both the native servlet response ones and the ones held by Spring's servlet response are added. Once the response is committed and headers written, reading headers from both yields duplicates. I think this is a bug uncovered by other changes in this PR.

}
boolean isEmpty1 = CollectionUtils.isEmpty(values1);

List<String> values2 = super.get(key);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright 2002-2016 the original author or authors.
* Copyright 2002-2018 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.
Expand All @@ -24,6 +24,7 @@

import org.springframework.core.io.buffer.DataBuffer;
import org.springframework.core.io.buffer.DataBufferFactory;
import org.springframework.http.HttpHeaders;

/**
* Abstract base class for listener-based server responses, e.g. Servlet 3.1
Expand All @@ -41,6 +42,10 @@ public AbstractListenerServerHttpResponse(DataBufferFactory dataBufferFactory) {
super(dataBufferFactory);
}

public AbstractListenerServerHttpResponse(DataBufferFactory dataBufferFactory, HttpHeaders headers) {
super(dataBufferFactory, headers);
}


@Override
protected final Mono<Void> writeWithInternal(Publisher<? extends DataBuffer> body) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,14 @@ private enum State {NEW, COMMITTING, COMMITTED}


public AbstractServerHttpResponse(DataBufferFactory dataBufferFactory) {
this(dataBufferFactory, new HttpHeaders());
}

public AbstractServerHttpResponse(DataBufferFactory dataBufferFactory, HttpHeaders headers) {
Assert.notNull(dataBufferFactory, "DataBufferFactory must not be null");
Assert.notNull(headers, "HttpHeaders must not be null");
this.dataBufferFactory = dataBufferFactory;
this.headers = new HttpHeaders();
this.headers = headers;
this.cookies = new LinkedMultiValueMap<>();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,7 @@ public DefaultServerHttpRequestBuilder(ServerHttpRequest original) {
this.httpMethodValue = original.getMethodValue();
this.body = original.getBody();

this.httpHeaders = new HttpHeaders();
copyMultiValueMap(original.getHeaders(), this.httpHeaders);
this.httpHeaders = HttpHeaders.writableHttpHeaders(original.getHeaders());

this.cookies = new LinkedMultiValueMap<>(original.getCookies().size());
copyMultiValueMap(original.getCookies(), this.cookies);
Expand Down
Loading