Skip to content

Commit

Permalink
Refine how async request state lock is obtained
Browse files Browse the repository at this point in the history 
Rather than waiting indefinitely, keep checking if the state
changed from ASYNC (e.g. to ERROR).

Closes gh-33421
  • Loading branch information
@rstoyanchev
rstoyanchev committed Sep 9, 2024
1 parent b7c7823 commit 3d1bf28
Showing 1 changed file with 52 additions and 45 deletions.
97 changes: 52 additions & 45 deletions ...in/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Consumer;

Expand Down Expand Up @@ -211,6 +212,38 @@ public void onComplete(AsyncEvent event) throws IOException {
}


/**
* Return 0 when there is no need to obtain a lock (no async handling in
* progress), 1 if lock was acquired, and -1 if lock is not acquired because
* request is no longer usable.
*/
private int tryObtainLock() {

if (this.state == State.NEW) {
return 0;
}

// Do not wait indefinitely, stop if we moved on from ASYNC state (e.g. to ERROR),
// helps to avoid ABBA deadlock with onError callback

while (this.state == State.ASYNC) {
try {
if (this.stateLock.tryLock(500, TimeUnit.MILLISECONDS)) {
if (this.state == State.ASYNC) {
return 1;
}
this.stateLock.unlock();
break;
}
}
catch (InterruptedException ex) {
// ignore
}
}

return -1;
}

/**
* Package private access for testing only.
*/
Expand Down Expand Up @@ -244,7 +277,7 @@ public void setAsyncWebRequest(StandardServletAsyncWebRequest asyncWebRequest) {

@Override
public ServletOutputStream getOutputStream() throws IOException {
int level = obtainLockAndCheckState();
int level = obtainLockOrRaiseException();
try {
if (this.outputStream == null) {
Assert.notNull(this.asyncWebRequest, "Not initialized");
Expand All @@ -263,7 +296,7 @@ public ServletOutputStream getOutputStream() throws IOException {

@Override
public PrintWriter getWriter() throws IOException {
int level = obtainLockAndCheckState();
int level = obtainLockOrRaiseException();
try {
if (this.writer == null) {
Assert.notNull(this.asyncWebRequest, "Not initialized");
Expand All @@ -281,7 +314,7 @@ public PrintWriter getWriter() throws IOException {

@Override
public void flushBuffer() throws IOException {
int level = obtainLockAndCheckState();
int level = obtainLockOrRaiseException();
try {
getResponse().flushBuffer();
}
Expand All @@ -293,25 +326,15 @@ public void flushBuffer() throws IOException {
}
}

/**
* Return 0 if checks passed and lock is not needed, 1 if checks passed
* and lock is held, or raise AsyncRequestNotUsableException.
*/
private int obtainLockAndCheckState() throws AsyncRequestNotUsableException {
private int obtainLockOrRaiseException() throws AsyncRequestNotUsableException {
Assert.notNull(this.asyncWebRequest, "Not initialized");
if (this.asyncWebRequest.state == State.NEW) {
return 0;
}

this.asyncWebRequest.stateLock.lock();
if (this.asyncWebRequest.state == State.ASYNC) {
return 1;
int result = this.asyncWebRequest.tryObtainLock();
if (result == -1) {
throw new AsyncRequestNotUsableException("Response not usable after " +
(this.asyncWebRequest.state == State.COMPLETED ?
"async request completion" : "response errors") + ".");
}

this.asyncWebRequest.stateLock.unlock();
throw new AsyncRequestNotUsableException("Response not usable after " +
(this.asyncWebRequest.state == State.COMPLETED ?
"async request completion" : "response errors") + ".");
return result;
}

void handleIOException(IOException ex, String msg) throws AsyncRequestNotUsableException {
Expand Down Expand Up @@ -357,7 +380,7 @@ public void setWriteListener(WriteListener writeListener) {

@Override
public void write(int b) throws IOException {
int level = this.response.obtainLockAndCheckState();
int level = this.response.obtainLockOrRaiseException();
try {
this.delegate.write(b);
}
Expand All @@ -370,7 +393,7 @@ public void write(int b) throws IOException {
}

public void write(byte[] buf, int offset, int len) throws IOException {
int level = this.response.obtainLockAndCheckState();
int level = this.response.obtainLockOrRaiseException();
try {
this.delegate.write(buf, offset, len);
}
Expand All @@ -384,7 +407,7 @@ public void write(byte[] buf, int offset, int len) throws IOException {

@Override
public void flush() throws IOException {
int level = this.response.obtainLockAndCheckState();
int level = this.response.obtainLockOrRaiseException();
try {
this.delegate.flush();
}
Expand All @@ -398,7 +421,7 @@ public void flush() throws IOException {

@Override
public void close() throws IOException {
int level = this.response.obtainLockAndCheckState();
int level = this.response.obtainLockOrRaiseException();
try {
this.delegate.close();
}
Expand Down Expand Up @@ -432,7 +455,7 @@ private LifecyclePrintWriter(PrintWriter delegate, StandardServletAsyncWebReques

@Override
public void flush() {
int level = tryObtainLockAndCheckState();
int level = this.asyncWebRequest.tryObtainLock();
if (level > -1) {
try {
this.delegate.flush();
Expand All @@ -445,7 +468,7 @@ public void flush() {

@Override
public void close() {
int level = tryObtainLockAndCheckState();
int level = this.asyncWebRequest.tryObtainLock();
if (level > -1) {
try {
this.delegate.close();
Expand All @@ -463,7 +486,7 @@ public boolean checkError() {

@Override
public void write(int c) {
int level = tryObtainLockAndCheckState();
int level = this.asyncWebRequest.tryObtainLock();
if (level > -1) {
try {
this.delegate.write(c);
Expand All @@ -476,7 +499,7 @@ public void write(int c) {

@Override
public void write(char[] buf, int off, int len) {
int level = tryObtainLockAndCheckState();
int level = this.asyncWebRequest.tryObtainLock();
if (level > -1) {
try {
this.delegate.write(buf, off, len);
Expand All @@ -494,7 +517,7 @@ public void write(char[] buf) {

@Override
public void write(String s, int off, int len) {
int level = tryObtainLockAndCheckState();
int level = this.asyncWebRequest.tryObtainLock();
if (level > -1) {
try {
this.delegate.write(s, off, len);
Expand All @@ -510,22 +533,6 @@ public void write(String s) {
this.delegate.write(s);
}

/**
* Return 0 if checks passed and lock is not needed, 1 if checks passed
* and lock is held, and -1 if checks did not pass.
*/
private int tryObtainLockAndCheckState() {
if (this.asyncWebRequest.state == State.NEW) {
return 0;
}
this.asyncWebRequest.stateLock.lock();
if (this.asyncWebRequest.state == State.ASYNC) {
return 1;
}
this.asyncWebRequest.stateLock.unlock();
return -1;
}

private void releaseLock(int level) {
if (level > 0) {
this.asyncWebRequest.stateLock.unlock();
Expand Down

0 comments on commit 3d1bf28

Please sign in to comment.