Skip to content

Commit a079570

Browse files
mtdowlingmtdowling
committed
Fix remaining SpotBugs findings in http-client
1 parent da1af60 commit a079570

6 files changed

Lines changed: 55 additions & 37 deletions

File tree

‎config/spotbugs/filter.xml‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -103,6 +103,25 @@
103103
<Class name="~.*\.http\.client\.VirtualThreadScalingBenchmark.*"/>
104104
</Match>
105105

106+
<!--
107+
~ http-client JMH benchmark harness (src/jmh) - not production code:
108+
~ - @AuxCounters Counter fields are written by the benchmark and read reflectively by JMH (UrF).
109+
~ - Shared payload constants are intentionally public so benchmarks in sibling packages reuse them (MS).
110+
~ - Benchmark teardown intentionally ignores close() errors (DE).
111+
-->
112+
<Match>
113+
<Class name="~software\.amazon\.smithy\.java\.http\.client\..*Benchmark(\$.*)?"/>
114+
<Bug pattern="URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD,UUF_UNUSED_PUBLIC_OR_PROTECTED_FIELD,MS_MUTABLE_ARRAY,MS_PKGPROTECT,MS_CANNOT_BE_FINAL"/>
115+
</Match>
116+
<Match>
117+
<Class name="software.amazon.smithy.java.http.client.BenchmarkSupport"/>
118+
<Bug pattern="MS_MUTABLE_ARRAY,MS_PKGPROTECT,MS_CANNOT_BE_FINAL"/>
119+
</Match>
120+
<Match>
121+
<Class name="~software\.amazon\.smithy\.java\.http\.client\.h2\.(EventLoopH2cTransport|ConnectionAgentH2cTransport|EventLoopH2Transport)"/>
122+
<Bug pattern="DE_MIGHT_IGNORE"/>
123+
</Match>
124+
106125
<!-- JMH reads @AuxCounters fields reflectively to report them as auxiliary counters. -->
107126
<Match>
108127
<Class name="software.amazon.smithy.java.aws.client.rulesengine.S3EndpointBenchmark$BytecodeMetrics" />

‎http/http-client/src/jmh/java/software/amazon/smithy/java/http/client/H2cMixedGetPutBenchmark.java‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
import org.openjdk.jmh.annotations.TearDown;
2929
import org.openjdk.jmh.annotations.Threads;
3030
import org.openjdk.jmh.annotations.Warmup;
31-
import software.amazon.smithy.java.context.Context;
3231
import software.amazon.smithy.java.http.api.HttpRequest;
3332
import software.amazon.smithy.java.http.client.connection.HttpVersionPolicy;
3433
import software.amazon.smithy.java.http.client.h2.ConnectionAgentH2cTransport;
@@ -60,7 +59,6 @@ public class H2cMixedGetPutBenchmark {
6059
private int streamsPerConnection;
6160

6261
private HttpClient smithyClient;
63-
private Context transportContext;
6462
private List<EventLoopH2cTransport> eventLoopTransports;
6563
private AtomicInteger eventLoopIndex;
6664
private List<ConnectionAgentH2cTransport> agentTransports;
@@ -78,7 +76,6 @@ public void setup() throws Exception {
7876
.httpVersionPolicy(HttpVersionPolicy.H2C_PRIOR_KNOWLEDGE)
7977
.dnsResolver(BenchmarkSupport.staticDns())
8078
.build();
81-
transportContext = Context.create();
8279
eventLoopTransports = new ArrayList<>(connections);
8380
for (int i = 0; i < connections; i++) {
8481
eventLoopTransports.add(new EventLoopH2cTransport(BenchmarkSupport.BENCH_HOST, BenchmarkSupport.H2C_PORT));

‎http/http-client/src/main/java/software/amazon/smithy/java/http/client/DefaultHttpClient.java‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,7 +182,9 @@ private HttpResponse sendForRoute(
182182
} catch (IOException e) {
183183
try {
184184
exchange.close();
185-
} catch (IOException ignored) {}
185+
} catch (IOException closeError) {
186+
LOGGER.debug("Error closing exchange after request failure: {}", closeError.getMessage());
187+
}
186188
connectionPool.evict(conn, true);
187189
// Do not fire onRequestEnd here: a per-route attempt failure may be retried on the next
188190
// proxy, and send() owns the single terminal failure event.
@@ -548,6 +550,8 @@ public void shutdown(Duration timeout) {
548550
executorService.shutdownNow();
549551
try {
550552
connectionPool.shutdown(timeout);
551-
} catch (IOException ignored) {}
553+
} catch (IOException e) {
554+
LOGGER.debug("Error shutting down connection pool: {}", e.getMessage());
555+
}
552556
}
553557
}

‎http/http-client/src/main/java/software/amazon/smithy/java/http/client/h1/H1Exchange.java‎

Lines changed: 23 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -79,6 +79,10 @@ final class H1Exchange implements HttpExchange {
7979
private String responseContentType;
8080
private long responseContentLength = -1;
8181
private boolean responseChunked;
82+
// Keep-alive override from the response Connection header: null = not specified (use protocol
83+
// default), TRUE = keep-alive, FALSE = close. Set by captureControlHeader alongside the other
84+
// response header fields.
85+
private Boolean responseKeepAlive;
8286
private int statusCode = -1;
8387
private boolean requestWritten = false;
8488
private boolean expectContinueHandled = false;
@@ -115,6 +119,7 @@ H1Exchange init(HttpRequest request) throws IOException {
115119
this.responseContentType = null;
116120
this.responseContentLength = -1;
117121
this.responseChunked = false;
122+
this.responseKeepAlive = null;
118123
this.statusCode = -1;
119124
this.requestWritten = false;
120125
this.expectContinueHandled = false;
@@ -607,7 +612,6 @@ private void parseStatusAndHeaders(int code, UnsyncBufferedInputStream in) throw
607612

608613
ModifiableHttpHeaders headers = HttpHeaders.ofModifiable();
609614
int headerCount = 0;
610-
Boolean keepAlive = null;
611615

612616
int lineLen;
613617
while ((lineLen = readLine(in)) > 0) {
@@ -625,52 +629,44 @@ private void parseStatusAndHeaders(int code, UnsyncBufferedInputStream in) throw
625629
int valueStart = H1Utils.headerValueStart(responseLineBuffer, colon, lineLen);
626630
int valueEnd = H1Utils.headerValueEnd(responseLineBuffer, valueStart, lineLen);
627631
String name = H1Utils.parseHeaderLine(responseLineBuffer, colon, valueStart, valueEnd, headers);
628-
Boolean keepAliveOverride = captureControlHeader(responseLineBuffer, valueStart, valueEnd, name);
629-
if (keepAliveOverride != null) {
630-
keepAlive = keepAliveOverride;
631-
}
632+
captureControlHeader(responseLineBuffer, valueStart, valueEnd, name);
632633
}
633634

634635
this.responseHeaders = headers;
635636

636-
if (keepAlive != null) {
637-
connection.setKeepAlive(keepAlive);
637+
if (responseKeepAlive != null) {
638+
connection.setKeepAlive(responseKeepAlive);
638639
}
639640
}
640641

641-
private Boolean captureControlHeader(byte[] line, int valueStart, int valueEnd, String name) throws IOException {
642-
return switch (name) {
642+
// Records the content-length, transfer-encoding, content-type, and connection (keep-alive) response
643+
// headers into the corresponding instance fields. Other headers are ignored here.
644+
private void captureControlHeader(byte[] line, int valueStart, int valueEnd, String name) throws IOException {
645+
switch (name) {
643646
case "content-length" -> {
644647
long length = parseContentLength(line, valueStart, valueEnd);
645648
if (responseContentLength >= 0 && responseContentLength != length) {
646649
throw new IOException("Conflicting Content-Length headers: "
647650
+ responseContentLength + " and " + length);
648651
}
649652
responseContentLength = length;
650-
yield null;
651-
}
652-
case "transfer-encoding" -> {
653-
responseChunked = containsChunked(line, valueStart, valueEnd);
654-
yield null;
655-
}
656-
case "content-type" -> {
657-
responseContentType = new String(
658-
line,
659-
valueStart,
660-
valueEnd - valueStart,
661-
StandardCharsets.US_ASCII);
662-
yield null;
663653
}
654+
case "transfer-encoding" -> responseChunked = containsChunked(line, valueStart, valueEnd);
655+
case "content-type" -> responseContentType = new String(
656+
line,
657+
valueStart,
658+
valueEnd - valueStart,
659+
StandardCharsets.US_ASCII);
664660
case "connection" -> {
665661
if (equalsIgnoreCase(line, valueStart, valueEnd, "close")) {
666-
yield Boolean.FALSE;
662+
responseKeepAlive = Boolean.FALSE;
667663
} else if (equalsIgnoreCase(line, valueStart, valueEnd, "keep-alive")) {
668-
yield Boolean.TRUE;
664+
responseKeepAlive = Boolean.TRUE;
669665
}
670-
yield null;
671666
}
672-
default -> null;
673-
};
667+
default -> {
668+
}
669+
}
674670
}
675671

676672
private static boolean isOWS(byte b) {

‎http/http-client/src/main/java/software/amazon/smithy/java/http/client/h2/FlowControlWindow.java‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,11 @@ int tryAcquireUpTo(int maxBytes, long timeoutMs) throws InterruptedException {
7676
if (remainingNs <= 0) {
7777
return 0;
7878
}
79-
available.awaitNanos(Math.min(remainingNs, POLL_INTERVAL_NS));
79+
// Return value intentionally ignored: the loop re-checks the window via
80+
// tryAcquireNonBlocking and recomputes the remaining time from deadlineNs, so the
81+
// nanos-left hint from awaitNanos adds nothing. A short POLL_INTERVAL_NS cap bounds
82+
// the wait so a missed release signal is still picked up on the next tick.
83+
long ignored = available.awaitNanos(Math.min(remainingNs, POLL_INTERVAL_NS));
8084
}
8185
} finally {
8286
lock.unlock();

‎http/http-client/src/main/java/software/amazon/smithy/java/http/client/h2/H2FrameCodec.java‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -395,10 +395,8 @@ byte[] readHeaderBlock(int initialStreamId, byte[] initialPayload, int initialLe
395395
if (initialPayload != null && initialLength > 0
396396
&& (currentType == FRAME_TYPE_HEADERS || currentType == FRAME_TYPE_PUSH_PROMISE)) {
397397
if ((initialFlags & FLAG_PADDED) != 0) {
398-
if (fragmentLength < 1) {
399-
throw new H2Exception(ERROR_FRAME_SIZE_ERROR,
400-
frameTypeName(currentType) + " padded frame missing pad-length byte");
401-
}
398+
// fragmentLength >= 1 is guaranteed by the initialLength > 0 guard above (fragmentLength
399+
// was initialized to initialLength), so the pad-length byte is always present here.
402400
int padLen = initialPayload[fragmentOffset] & 0xFF;
403401
fragmentOffset++;
404402
fragmentLength--;

0 commit comments

Comments
 (0)