From 3d1bf28445e80ff8428d62556560143275f1e548 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Thu, 5 Sep 2024 15:15:29 +0200 Subject: [PATCH 1/3] Refine how async request state lock is obtained Rather than waiting indefinitely, keep checking if the state changed from ASYNC (e.g. to ERROR). Closes gh-33421 --- .../async/StandardServletAsyncWebRequest.java | 97 ++++++++++--------- 1 file changed, 52 insertions(+), 45 deletions(-) diff --git a/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java b/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java index b8f767c577..96dc905c53 100644 --- a/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java +++ b/spring-web/src/main/java/org/springframework/web/context/request/async/StandardServletAsyncWebRequest.java @@ -21,6 +21,7 @@ import java.io.PrintWriter; 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; @@ -211,6 +212,38 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements } + /** + * 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. */ @@ -244,7 +277,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public ServletOutputStream getOutputStream() throws IOException { - int level = obtainLockAndCheckState(); + int level = obtainLockOrRaiseException(); try { if (this.outputStream == null) { Assert.notNull(this.asyncWebRequest, "Not initialized"); @@ -263,7 +296,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public PrintWriter getWriter() throws IOException { - int level = obtainLockAndCheckState(); + int level = obtainLockOrRaiseException(); try { if (this.writer == null) { Assert.notNull(this.asyncWebRequest, "Not initialized"); @@ -281,7 +314,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void flushBuffer() throws IOException { - int level = obtainLockAndCheckState(); + int level = obtainLockOrRaiseException(); try { getResponse().flushBuffer(); } @@ -293,25 +326,15 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements } } - /** - * 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; + 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.lock(); - if (this.asyncWebRequest.state == State.ASYNC) { - return 1; - } - - 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 { @@ -357,7 +380,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void write(int b) throws IOException { - int level = this.response.obtainLockAndCheckState(); + int level = this.response.obtainLockOrRaiseException(); try { this.delegate.write(b); } @@ -370,7 +393,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements } 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); } @@ -384,7 +407,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void flush() throws IOException { - int level = this.response.obtainLockAndCheckState(); + int level = this.response.obtainLockOrRaiseException(); try { this.delegate.flush(); } @@ -398,7 +421,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void close() throws IOException { - int level = this.response.obtainLockAndCheckState(); + int level = this.response.obtainLockOrRaiseException(); try { this.delegate.close(); } @@ -432,7 +455,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void flush() { - int level = tryObtainLockAndCheckState(); + int level = this.asyncWebRequest.tryObtainLock(); if (level > -1) { try { this.delegate.flush(); @@ -445,7 +468,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void close() { - int level = tryObtainLockAndCheckState(); + int level = this.asyncWebRequest.tryObtainLock(); if (level > -1) { try { this.delegate.close(); @@ -463,7 +486,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @Override public void write(int c) { - int level = tryObtainLockAndCheckState(); + int level = this.asyncWebRequest.tryObtainLock(); if (level > -1) { try { this.delegate.write(c); @@ -476,7 +499,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @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); @@ -494,7 +517,7 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements @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); @@ -510,22 +533,6 @@ public class StandardServletAsyncWebRequest extends ServletWebRequest implements 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(); From 76553294634a6cfb071ad90fbddffdb51df2bf37 Mon Sep 17 00:00:00 2001 From: yfoelsin Date: Fri, 6 Sep 2024 15:15:29 +0200 Subject: [PATCH 2/3] Set status code on DefaultRenderingBuilder for RedirectView See gh-33498 --- .../reactive/result/view/DefaultRenderingBuilder.java | 3 +++ .../result/view/DefaultRenderingBuilderTests.java | 9 +++++++++ 2 files changed, 12 insertions(+) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java index c1d0923e97..4d452cd127 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java @@ -85,6 +85,9 @@ class DefaultRenderingBuilder implements Rendering.RedirectBuilder { @Override public DefaultRenderingBuilder status(HttpStatusCode status) { this.status = status; + if (this.view instanceof RedirectView) { + ((RedirectView) this.view).setStatusCode(status); + } return this; } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java index 1c33c68808..f96ab31a00 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java @@ -126,6 +126,15 @@ class DefaultRenderingBuilderTests { assertThat(((RedirectView) view).isPropagateQuery()).isTrue(); } + @Test + void redirectWithCustomStatus() { + Rendering rendering = Rendering.redirectTo("foo").status(HttpStatus.MOVED_PERMANENTLY).build(); + + Object view = rendering.view(); + assertThat(view.getClass()).isEqualTo(RedirectView.class); + assertThat(((RedirectView) view).statusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY); + } + private static class Foo {} From 5c1ab7ecd5b05c1df208567d707338616fc293e5 Mon Sep 17 00:00:00 2001 From: rstoyanchev Date: Fri, 6 Sep 2024 17:02:33 +0200 Subject: [PATCH 3/3] Polishing contribution Closes gh-33498 --- .../result/view/DefaultRenderingBuilder.java | 6 ++++-- .../result/view/DefaultRenderingBuilderTests.java | 8 +++++--- .../view/ViewResolutionResultHandlerTests.java | 15 +++++++++++++++ 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java index 4d452cd127..9f7402fc26 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilder.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2024 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -84,10 +84,12 @@ class DefaultRenderingBuilder implements Rendering.RedirectBuilder { @Override public DefaultRenderingBuilder status(HttpStatusCode status) { - this.status = status; if (this.view instanceof RedirectView) { ((RedirectView) this.view).setStatusCode(status); } + else { + this.status = status; + } return this; } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java index f96ab31a00..7b97e8eab6 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/DefaultRenderingBuilderTests.java @@ -23,6 +23,7 @@ import java.util.Map; import org.junit.jupiter.api.Test; import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; import static org.assertj.core.api.Assertions.assertThat; @@ -126,13 +127,14 @@ class DefaultRenderingBuilderTests { assertThat(((RedirectView) view).isPropagateQuery()).isTrue(); } - @Test + @Test // gh-33498 void redirectWithCustomStatus() { - Rendering rendering = Rendering.redirectTo("foo").status(HttpStatus.MOVED_PERMANENTLY).build(); + HttpStatus status = HttpStatus.MOVED_PERMANENTLY; + Rendering rendering = Rendering.redirectTo("foo").status(status).build(); Object view = rendering.view(); assertThat(view.getClass()).isEqualTo(RedirectView.class); - assertThat(((RedirectView) view).statusCode()).isEqualTo(HttpStatus.MOVED_PERMANENTLY); + assertThat(((RedirectView) view).getStatusCode()).isEqualTo(status); } diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandlerTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandlerTests.java index f08d538121..f30575d669 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandlerTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ViewResolutionResultHandlerTests.java @@ -16,6 +16,7 @@ package org.springframework.web.reactive.result.view; +import java.net.URI; import java.nio.ByteBuffer; import java.time.Duration; import java.util.Arrays; @@ -196,6 +197,20 @@ class ViewResolutionResultHandlerTests { assertThat(exchange.getResponse().getHeaders().getFirst("h")).isEqualTo("h1"); } + @Test // gh-33498 + void handleRedirect() { + HttpStatus status = HttpStatus.MOVED_PERMANENTLY; + Rendering returnValue = Rendering.redirectTo("foo").status(status).build(); + MethodParameter returnType = on(Handler.class).resolveReturnType(Rendering.class); + HandlerResult result = new HandlerResult(new Object(), returnValue, returnType, this.bindingContext); + + MockServerWebExchange exchange = MockServerWebExchange.from(get("/path")); + resultHandler(new UrlBasedViewResolver()).handleResult(exchange, result).block(Duration.ofSeconds(5)); + + assertThat(exchange.getResponse().getStatusCode()).isEqualTo(status); + assertThat(exchange.getResponse().getHeaders().getLocation()).isEqualTo(URI.create("foo")); + } + @Test void handleWithMultipleResolvers() { testHandle("/account",