Skip to content

Commit

Permalink
Fix potential hang with concurrent reads or concurrent writes
Browse files Browse the repository at this point in the history
Fot two threads, T1 and T2, both writing to the same connection, the
sequence of events for a failure is as follows (line numbers all pre
this commit):

- T1 obtains the write semaphore (L1366)
- T1 creates an OperationState and sets writeOperation (L1390)
- the async write for T1 completes and the completion handler is called
- T1's completion handler releases the semaphore (L1046)
- T2 obtains the write semaphore (L1366)
- T2 creates an OperationState and sets writeOperation (L1390)
- T1's completion handler clears writeOperation (L1050)
- the async write for T2 does not complete and the socket is added to
  the Poller
- The Poller signals the socket is ready for write
- The Poller finds writeOperation is null so performs a normal dispatch
  for write
- The async write times out as it never receives the notification from
  the Poller

The fix is to swap the order of clearing writeOperation and releasing
the semaphore.
  • Loading branch information
markt-asf committed Jun 17, 2021
1 parent c87fd5f commit 92b9185
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 2 deletions.
12 changes: 10 additions & 2 deletions java/org/apache/tomcat/util/net/SocketWrapperBase.java
Original file line number Diff line number Diff line change
Expand Up @@ -1043,12 +1043,16 @@ public void completed(Long nBytes, OperationState<A> state) {
}
if (complete) {
boolean notify = false;
state.semaphore.release();
if (state.read) {
readOperation = null;
} else {
writeOperation = null;
}
// Semaphore must be released after [read|write]Operation is cleared
// to ensure that the next thread to hold the semaphore hasn't
// written a new value to [read|write]Operation by the time it is
// cleared.
state.semaphore.release();
if (state.block == BlockingMode.BLOCK && currentState != CompletionState.INLINE) {
notify = true;
} else {
Expand Down Expand Up @@ -1084,12 +1088,16 @@ public void failed(Throwable exc, OperationState<A> state) {
}
setError(ioe);
boolean notify = false;
state.semaphore.release();
if (state.read) {
readOperation = null;
} else {
writeOperation = null;
}
// Semaphore must be released after [read|write]Operation is cleared
// to ensure that the next thread to hold the semaphore hasn't
// written a new value to [read|write]Operation by the time it is
// cleared.
state.semaphore.release();
if (state.block == BlockingMode.BLOCK) {
notify = true;
} else {
Expand Down
5 changes: 5 additions & 0 deletions webapps/docs/changelog.xml
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,11 @@
buffer is now flushed (and the response committed if required) if the
buffer is full and there is more data to write. (markt)
</fix>
<fix>
Fix an issue where concurrent HTTP/2 writes (or concurrent reads) to the
same connection could hang and eventually timeout when async IO was
enabled (it is enabled by default). (markt)
</fix>
</changelog>
</subsection>
<subsection name="Web applications">
Expand Down

0 comments on commit 92b9185

Please sign in to comment.