This is an automated email from the ASF dual-hosted git repository.
markt pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new f53500aa28 Fix various SpotBugs warnings
f53500aa28 is described below
commit f53500aa28c15921c0d4545a1d6c338dc1b5a91c
Author: Mark Thomas <[email protected]>
AuthorDate: Thu Apr 13 14:36:01 2023 +0100
Fix various SpotBugs warnings
---
.../apache/coyote/http2/Http2UpgradeHandler.java | 21 +++++++++++++--------
java/org/apache/tomcat/websocket/WsSession.java | 6 ++++--
res/spotbugs/filter-false-positives.xml | 21 +++++++++++++++++++++
.../tomcat/websocket/server/TesterWsClient.java | 12 ++++++++++--
4 files changed, 48 insertions(+), 12 deletions(-)
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index 6e0343689f..8f9366daec 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -32,6 +32,7 @@ import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicLong;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.Lock;
import jakarta.servlet.ServletConnection;
import jakarta.servlet.http.WebConnection;
@@ -682,7 +683,8 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
byte[] payloadLength = new byte[3];
ByteUtil.setThreeBytes(payloadLength, 0, len);
- socketWrapper.getLock().lock();
+ Lock lock = socketWrapper.getLock();
+ lock.lock();
try {
socketWrapper.write(true, payloadLength, 0, payloadLength.length);
socketWrapper.write(true, GOAWAY, 0, GOAWAY.length);
@@ -692,18 +694,19 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
}
socketWrapper.flush(true);
} finally {
- socketWrapper.getLock().unlock();
+ lock.unlock();
}
}
void writeHeaders(Stream stream, int pushedStreamId, MimeHeaders
mimeHeaders, boolean endOfStream, int payloadSize)
throws IOException {
// This ensures the Stream processing thread has control of the socket.
- socketWrapper.getLock().lock();
+ Lock lock = socketWrapper.getLock();
+ lock.lock();
try {
doWriteHeaders(stream, pushedStreamId, mimeHeaders, endOfStream,
payloadSize);
} finally {
- socketWrapper.getLock().unlock();
+ lock.unlock();
}
stream.sentHeaders();
if (endOfStream) {
@@ -906,7 +909,8 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
protected void processWrites() throws IOException {
- socketWrapper.getLock().lock();
+ Lock lock = socketWrapper.getLock();
+ lock.lock();
try {
if (socketWrapper.flush(false)) {
socketWrapper.registerWriteInterest();
@@ -916,7 +920,7 @@ class Http2UpgradeHandler extends AbstractStream implements
InternalHttpUpgradeH
pingManager.sendPing(false);
}
} finally {
- socketWrapper.getLock().unlock();
+ lock.unlock();
}
}
@@ -1337,13 +1341,14 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
// Synchronized since PUSH_PROMISE frames have to be sent in order.
Once
// the stream has been created we need to ensure that the PUSH_PROMISE
// is sent before the next stream is created for a PUSH_PROMISE.
- socketWrapper.getLock().lock();
+ Lock lock = socketWrapper.getLock();
+ lock.lock();
try {
pushStream = createLocalStream(request);
writeHeaders(associatedStream, pushStream.getIdAsInt(),
request.getMimeHeaders(), false,
Constants.DEFAULT_HEADERS_FRAME_SIZE);
} finally {
- socketWrapper.getLock().unlock();
+ lock.unlock();
}
pushStream.sentPushPromise();
diff --git a/java/org/apache/tomcat/websocket/WsSession.java
b/java/org/apache/tomcat/websocket/WsSession.java
index a3310eec0d..8aad5a70ff 100644
--- a/java/org/apache/tomcat/websocket/WsSession.java
+++ b/java/org/apache/tomcat/websocket/WsSession.java
@@ -28,6 +28,7 @@ import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.atomic.AtomicLong;
+import java.util.concurrent.locks.Lock;
import javax.naming.NamingException;
@@ -619,7 +620,8 @@ public class WsSession implements Session {
*/
public void onClose(CloseReason closeReason) {
- wsRemoteEndpoint.getLock().lock();
+ Lock lock = wsRemoteEndpoint.getLock();
+ lock.lock();
try {
if (state != State.CLOSED) {
try {
@@ -639,7 +641,7 @@ public class WsSession implements Session {
wsRemoteEndpoint.close();
}
} finally {
- wsRemoteEndpoint.getLock().unlock();
+ lock.unlock();
}
}
diff --git a/res/spotbugs/filter-false-positives.xml
b/res/spotbugs/filter-false-positives.xml
index ac59de62b1..f0250f888c 100644
--- a/res/spotbugs/filter-false-positives.xml
+++ b/res/spotbugs/filter-false-positives.xml
@@ -1774,6 +1774,15 @@
<Field name="buffers" />
<Bug pattern="VO_VOLATILE_REFERENCE_TO_ARRAY" />
</Match>
+ <Match>
+ <!-- Locks are released then re-obtained which is the opposite order to
normal. -->
+ <Class
name="org.apache.tomcat.websocket.server.WsRemoteEndpointImplServer" />
+ <Method name="acquireMessagePartInProgressSemaphore" />
+ <Or>
+ <Bug pattern="UL_UNRELEASED_LOCK" />
+ <Bug pattern="UL_UNRELEASED_LOCK_EXCEPTION_PATH" />
+ </Or>
+ </Match>
<!-- Example code -->
<Match>
@@ -2566,6 +2575,18 @@
<Bug pattern="UW_UNCOND_WAIT " />
</Or>
</Match>
+ <Match>
+ <!-- Statics are used deliberately as they are simpler -->
+ <Class
name="org.apache.tomcat.websocket.server.TestWsRemoteEndpointImplServerDeadlock"
/>
+ <Method name="testTemporaryDeadlockOnClientClose" />
+ <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD"/>
+ </Match>
+ <Match>
+ <!-- Statics are used deliberately as they are simpler -->
+ <Class
name="org.apache.tomcat.websocket.server.TestWsRemoteEndpointImplServerDeadlock$Bug66508Endpoint"
/>
+ <Method name="onOpen" />
+ <Bug pattern="ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD"/>
+ </Match>
<Match>
<!-- Return value of latch is intentionally ignored -->
<Or>
diff --git a/test/org/apache/tomcat/websocket/server/TesterWsClient.java
b/test/org/apache/tomcat/websocket/server/TesterWsClient.java
index 1fc6a6ff73..ed52cee593 100644
--- a/test/org/apache/tomcat/websocket/server/TesterWsClient.java
+++ b/test/org/apache/tomcat/websocket/server/TesterWsClient.java
@@ -74,10 +74,18 @@ public class TesterWsClient {
public int readUpgradeResponse() throws IOException {
BufferedReader in = new BufferedReader(new
InputStreamReader(socket.getInputStream()));
+ int result = -1;
String line = in.readLine();
- // line expected to be "HTTP/1.1 nnn "
- int result = Integer.parseInt(line.substring(9, 12));
while (line != null && !line.isEmpty()) {
+ if (result == -1) {
+ if (line.length() > 11) {
+ // First line expected to be "HTTP/1.1 nnn "
+ result = Integer.parseInt(line.substring(9, 12));
+ } else {
+ // No response code - treat as server error for this test
+ result = 500;
+ }
+ }
line = in.readLine();
}
return result;
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
