Skip to content

Commit f25066a

Browse files
mtdowlingmtdowling
committed
Improve HttpHeaders
Better immutability No need to make mutable headers thread-safe. These are typically created in a single thread and then used after that. Use external synchronization if needed.
1 parent d006b04 commit f25066a

7 files changed

Lines changed: 111 additions & 34 deletions

File tree

‎http/http-api/src/main/java/software/amazon/smithy/java/http/api/HttpRequestImpl.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ private void beforeBuild() {
9898
Objects.requireNonNull(method, "Method cannot be null");
9999
Objects.requireNonNull(uri, "URI cannot be null");
100100
body = Objects.requireNonNullElse(body, DataStream.ofEmpty());
101+
mutatedHeaders = null; // decouple from built request
101102
}
102103

103104
@Override

‎http/http-api/src/main/java/software/amazon/smithy/java/http/api/HttpResponseImpl.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,7 @@ private void beforeBuild() {
8787
}
8888
Objects.requireNonNull(httpVersion);
8989
body = Objects.requireNonNullElse(body, DataStream.ofEmpty());
90+
mutatedHeaders = null; // decouple from built response
9091
}
9192

9293
@Override

‎http/http-api/src/main/java/software/amazon/smithy/java/http/api/ModifiableHttpHeaders.java‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@
1010

1111
/**
1212
* A modifiable version of {@link HttpHeaders}.
13+
*
14+
* <p><b>Thread Safety:</b> Implementations are <b>not</b> guaranteed to be thread-safe.
15+
* If multiple threads access an instance concurrently, and at least one thread modifies
16+
* the headers, external synchronization is required.
1317
*/
1418
public interface ModifiableHttpHeaders extends HttpHeaders {
1519
/**
@@ -97,6 +101,11 @@ default void setHeaders(Map<String, List<String>> headers) {
97101
*/
98102
void removeHeader(String name);
99103

104+
/**
105+
* Removes all headers.
106+
*/
107+
void clear();
108+
100109
@Override
101110
default ModifiableHttpHeaders toModifiable() {
102111
return this;

‎http/http-api/src/main/java/software/amazon/smithy/java/http/api/SimpleModifiableHttpHeaders.java‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -7,31 +7,48 @@
77

88
import java.util.ArrayList;
99
import java.util.Collections;
10+
import java.util.HashMap;
1011
import java.util.Iterator;
1112
import java.util.List;
1213
import java.util.Locale;
1314
import java.util.Map;
1415
import java.util.Objects;
15-
import java.util.concurrent.ConcurrentHashMap;
1616

17+
/**
18+
* Simple mutable HTTP headers implementation.
19+
*
20+
* <p><b>Thread Safety:</b> This class is <b>not</b> thread-safe. If multiple threads
21+
* access an instance concurrently, and at least one thread modifies the headers,
22+
* external synchronization is required.
23+
*/
1724
final class SimpleModifiableHttpHeaders implements ModifiableHttpHeaders {
1825

19-
private final Map<String, List<String>> headers = new ConcurrentHashMap<>();
26+
private final Map<String, List<String>> headers = new HashMap<>();
2027

2128
@Override
2229
public void addHeader(String name, String value) {
23-
headers.computeIfAbsent(formatPutKey(name), k -> new ArrayList<>()).add(value);
30+
getOrCreateValues(name).add(value);
2431
}
2532

2633
@Override
2734
public void addHeader(String name, List<String> values) {
28-
headers.computeIfAbsent(formatPutKey(name), k -> new ArrayList<>()).addAll(values);
35+
getOrCreateValues(name).addAll(values);
36+
}
37+
38+
private List<String> getOrCreateValues(String name) {
39+
var key = formatPutKey(name);
40+
var values = headers.get(key);
41+
if (values == null) {
42+
values = new ArrayList<>();
43+
headers.put(key, values);
44+
}
45+
return values;
2946
}
3047

3148
@Override
3249
public void setHeader(String name, String value) {
3350
var key = formatPutKey(name);
34-
var list = headers.get(formatPutKey(name));
51+
var list = headers.get(name);
3552
if (list == null) {
3653
list = new ArrayList<>(1);
3754
headers.put(key, list);
@@ -45,7 +62,7 @@ public void setHeader(String name, String value) {
4562
@Override
4663
public void setHeader(String name, List<String> values) {
4764
var key = formatPutKey(name);
48-
var list = headers.get(formatPutKey(name));
65+
var list = headers.get(name);
4966
if (list == null) {
5067
list = new ArrayList<>(values.size());
5168
headers.put(key, list);
@@ -69,6 +86,11 @@ public void removeHeader(String name) {
6986
headers.remove(name.toLowerCase(Locale.ENGLISH));
7087
}
7188

89+
@Override
90+
public void clear() {
91+
headers.clear();
92+
}
93+
7294
@Override
7395
public List<String> allValues(String name) {
7496
return headers.getOrDefault(name.toLowerCase(Locale.ENGLISH), Collections.emptyList());

‎http/http-api/src/main/java/software/amazon/smithy/java/http/api/SimpleUnmodifiableHttpHeaders.java‎

Lines changed: 62 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -26,31 +26,53 @@ final class SimpleUnmodifiableHttpHeaders implements HttpHeaders {
2626
SimpleUnmodifiableHttpHeaders(Map<String, List<String>> input, boolean copyHeaders) {
2727
if (!copyHeaders) {
2828
this.headers = input;
29+
} else if (input.isEmpty()) {
30+
this.headers = Collections.emptyMap();
2931
} else {
30-
// Ensure map keys are normalized to use lower-case header names.
31-
this.headers = new HashMap<>(input.size());
32+
// Single pass to normalize, trim, and make immutable in one go
33+
Map<String, List<String>> result = HashMap.newHashMap(input.size());
3234
for (var entry : input.entrySet()) {
33-
var key = entry.getKey().trim().toLowerCase(Locale.ENGLISH);
34-
headers.computeIfAbsent(key, k -> new ArrayList<>()).addAll(copyAndTrimValues(entry.getValue()));
35+
var key = normalizeKey(entry.getKey());
36+
var values = entry.getValue();
37+
var existing = result.get(key);
38+
if (existing == null) {
39+
existing = new ArrayList<>();
40+
result.put(key, existing);
41+
}
42+
copyAndTrimValuesInto(values, existing);
3543
}
36-
// Make the value immutable.
37-
for (var entry : headers.entrySet()) {
38-
entry.setValue(Collections.unmodifiableList(entry.getValue()));
44+
// make immutable lists
45+
for (var e : result.entrySet()) {
46+
e.setValue(Collections.unmodifiableList(e.getValue()));
3947
}
48+
this.headers = result;
4049
}
4150
}
4251

43-
private static List<String> copyAndTrimValues(List<String> source) {
44-
List<String> trimmedValues = new ArrayList<>(source.size());
45-
for (var value : source) {
46-
trimmedValues.add(value.trim());
52+
private static String normalizeKey(String key) {
53+
return key.trim().toLowerCase(Locale.ENGLISH);
54+
}
55+
56+
private static List<String> copyAndTrimValuesMutable(List<String> source) {
57+
int size = source.size();
58+
if (size == 0) {
59+
return new ArrayList<>(4);
60+
}
61+
var result = new ArrayList<String>(size);
62+
copyAndTrimValuesInto(source, result);
63+
return result;
64+
}
65+
66+
private static void copyAndTrimValuesInto(List<String> source, List<String> dest) {
67+
for (String s : source) {
68+
dest.add(s.trim());
4769
}
48-
return trimmedValues;
4970
}
5071

5172
@Override
5273
public List<String> allValues(String name) {
53-
return headers.getOrDefault(name.toLowerCase(Locale.ENGLISH), Collections.emptyList());
74+
var values = headers.get(name.toLowerCase(Locale.ENGLISH));
75+
return values != null ? values : List.of();
5476
}
5577

5678
@Override
@@ -76,7 +98,7 @@ public Map<String, List<String>> map() {
7698
@Override
7799
public ModifiableHttpHeaders toModifiable() {
78100
var mod = new SimpleModifiableHttpHeaders();
79-
Map<String, List<String>> copy = new HashMap<>(headers.size());
101+
Map<String, List<String>> copy = HashMap.newHashMap(headers.size());
80102
for (var entry : headers.entrySet()) {
81103
copy.put(entry.getKey(), new ArrayList<>(entry.getValue()));
82104
}
@@ -88,12 +110,10 @@ public ModifiableHttpHeaders toModifiable() {
88110
public boolean equals(Object obj) {
89111
if (obj == this) {
90112
return true;
91-
} else if (!(obj instanceof HttpHeaders)) {
113+
}
114+
if (!(obj instanceof HttpHeaders other)) {
92115
return false;
93116
}
94-
95-
// For unmodifiable headers, we treat mutable implementations the same.
96-
var other = (HttpHeaders) obj;
97117
return headers.equals(other.map());
98118
}
99119

@@ -125,8 +145,14 @@ static Map<String, List<String>> addHeaders(
125145
mutatedHeaders = copyHeaders(original.map());
126146
}
127147
for (var entry : from.entrySet()) {
128-
mutatedHeaders.computeIfAbsent(entry.getKey(), k -> new ArrayList<>())
129-
.addAll(copyAndTrimValues(entry.getValue()));
148+
var key = normalizeKey(entry.getKey());
149+
var list = mutatedHeaders.get(key);
150+
if (list == null) {
151+
list = copyAndTrimValuesMutable(entry.getValue());
152+
mutatedHeaders.put(key, list);
153+
} else {
154+
copyAndTrimValuesInto(entry.getValue(), list);
155+
}
130156
}
131157
return mutatedHeaders;
132158
}
@@ -138,18 +164,26 @@ static Map<String, List<String>> addHeader(
138164
String value
139165
) {
140166
if (mutatedHeaders == null) {
141-
mutatedHeaders = SimpleUnmodifiableHttpHeaders.copyHeaders(original.map());
167+
mutatedHeaders = copyHeaders(original.map());
142168
}
143-
field = field.toLowerCase(Locale.ENGLISH).trim();
169+
field = normalizeKey(field);
144170
value = value.trim();
145-
mutatedHeaders.computeIfAbsent(field, k -> new ArrayList<>()).add(value);
171+
var list = mutatedHeaders.get(field);
172+
if (list == null) {
173+
list = new ArrayList<>(4);
174+
mutatedHeaders.put(field, list);
175+
}
176+
list.add(value);
146177
return mutatedHeaders;
147178
}
148179

149180
static Map<String, List<String>> copyHeaders(Map<String, List<String>> from) {
150-
Map<String, List<String>> into = new HashMap<>(from.size());
181+
if (from.isEmpty()) {
182+
return new HashMap<>(8);
183+
}
184+
Map<String, List<String>> into = HashMap.newHashMap(from.size());
151185
for (var entry : from.entrySet()) {
152-
into.put(entry.getKey().toLowerCase(Locale.ENGLISH).trim(), copyAndTrimValues(entry.getValue()));
186+
into.put(normalizeKey(entry.getKey()), copyAndTrimValuesMutable(entry.getValue()));
153187
}
154188
return into;
155189
}
@@ -160,10 +194,10 @@ static Map<String, List<String>> replaceHeaders(
160194
Map<String, List<String>> replace
161195
) {
162196
if (mutated == null) {
163-
mutated = SimpleUnmodifiableHttpHeaders.copyHeaders(original.map());
197+
mutated = copyHeaders(original.map());
164198
}
165-
for (Map.Entry<String, List<String>> entry : replace.entrySet()) {
166-
mutated.put(entry.getKey().toLowerCase(Locale.ENGLISH).trim(), copyAndTrimValues(entry.getValue()));
199+
for (var entry : replace.entrySet()) {
200+
mutated.put(normalizeKey(entry.getKey()), copyAndTrimValuesMutable(entry.getValue()));
167201
}
168202
return mutated;
169203
}

‎server/server-core/src/test/java/software/amazon/smithy/java/server/core/TestStructs.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,11 @@ public void removeHeader(String name) {
154154
headers.remove(name);
155155
}
156156

157+
@Override
158+
public void clear() {
159+
headers.clear();
160+
}
161+
157162
@Override
158163
public List<String> allValues(String name) {
159164
return headers.getOrDefault(name, List.of());

‎server/server-netty/src/main/java/software/amazon/smithy/java/server/netty/NettyHttpHeaders.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,11 @@ public void removeHeader(String name) {
6262
nettyHeaders.remove(name);
6363
}
6464

65+
@Override
66+
public void clear() {
67+
nettyHeaders.clear();
68+
}
69+
6570
@Override
6671
public boolean isEmpty() {
6772
return nettyHeaders.isEmpty();

0 commit comments

Comments
 (0)