diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageReaderArgumentResolver.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageReaderArgumentResolver.java index 61d6bac1f7c..cfec9834baf 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageReaderArgumentResolver.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageReaderArgumentResolver.java @@ -35,6 +35,7 @@ import org.springframework.core.annotation.AnnotationUtils; import org.springframework.core.codec.DecodingException; import org.springframework.core.codec.Hints; import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.core.io.buffer.DataBufferUtils; import org.springframework.http.HttpMethod; import org.springframework.http.MediaType; import org.springframework.http.codec.HttpMessageReader; @@ -200,7 +201,8 @@ public abstract class AbstractMessageReaderArgumentResolver extends HandlerMetho HttpMethod method = request.getMethod(); if (contentType == null && method != null && SUPPORTED_METHODS.contains(method)) { - Flux body = request.getBody().doOnNext(o -> { + Flux body = request.getBody().doOnNext(buffer -> { + DataBufferUtils.release(buffer); // Body not empty, back to 415.. throw new UnsupportedMediaTypeStatusException(mediaType, this.supportedMediaTypes, elementType); }); diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerView.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerView.java index abce0b19ef1..399c6e47fc8 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerView.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerView.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2019 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. @@ -32,7 +32,6 @@ import freemarker.template.ObjectWrapper; import freemarker.template.SimpleHash; import freemarker.template.Template; import freemarker.template.Version; -import reactor.core.publisher.Flux; import reactor.core.publisher.Mono; import org.springframework.beans.BeansException; @@ -42,6 +41,7 @@ import org.springframework.context.ApplicationContextException; import org.springframework.context.i18n.LocaleContextHolder; import org.springframework.core.io.buffer.DataBuffer; import org.springframework.core.io.buffer.DataBufferUtils; +import org.springframework.core.io.buffer.PooledDataBuffer; import org.springframework.http.MediaType; import org.springframework.lang.Nullable; import org.springframework.util.Assert; @@ -184,30 +184,34 @@ public class FreeMarkerView extends AbstractUrlBasedView { protected Mono renderInternal(Map renderAttributes, @Nullable MediaType contentType, ServerWebExchange exchange) { - // Expose all standard FreeMarker hash models. - SimpleHash freeMarkerModel = getTemplateModel(renderAttributes, exchange); + return exchange.getResponse().writeWith(Mono + .fromCallable(() -> { + // Expose all standard FreeMarker hash models. + SimpleHash freeMarkerModel = getTemplateModel(renderAttributes, exchange); - if (logger.isDebugEnabled()) { - logger.debug(exchange.getLogPrefix() + "Rendering [" + getUrl() + "]"); - } + if (logger.isDebugEnabled()) { + logger.debug(exchange.getLogPrefix() + "Rendering [" + getUrl() + "]"); + } - Locale locale = LocaleContextHolder.getLocale(exchange.getLocaleContext()); - DataBuffer dataBuffer = exchange.getResponse().bufferFactory().allocateBuffer(); - try { - Charset charset = getCharset(contentType); - Writer writer = new OutputStreamWriter(dataBuffer.asOutputStream(), charset); - getTemplate(locale).process(freeMarkerModel, writer); - } - catch (IOException ex) { - DataBufferUtils.release(dataBuffer); - String message = "Could not load FreeMarker template for URL [" + getUrl() + "]"; - return Mono.error(new IllegalStateException(message, ex)); - } - catch (Throwable ex) { - DataBufferUtils.release(dataBuffer); - return Mono.error(ex); - } - return exchange.getResponse().writeWith(Flux.just(dataBuffer)); + Locale locale = LocaleContextHolder.getLocale(exchange.getLocaleContext()); + DataBuffer dataBuffer = exchange.getResponse().bufferFactory().allocateBuffer(); + try { + Charset charset = getCharset(contentType); + Writer writer = new OutputStreamWriter(dataBuffer.asOutputStream(), charset); + getTemplate(locale).process(freeMarkerModel, writer); + return dataBuffer; + } + catch (IOException ex) { + DataBufferUtils.release(dataBuffer); + String message = "Could not load FreeMarker template for URL [" + getUrl() + "]"; + throw new IllegalStateException(message, ex); + } + catch (Throwable ex) { + DataBufferUtils.release(dataBuffer); + throw ex; + } + }) + .doOnDiscard(PooledDataBuffer.class, DataBufferUtils::release)); } private Charset getCharset(@Nullable MediaType mediaType) { diff --git a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/script/ScriptTemplateView.java b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/script/ScriptTemplateView.java index d5df366fc4c..98131de7751 100644 --- a/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/script/ScriptTemplateView.java +++ b/spring-webflux/src/main/java/org/springframework/web/reactive/result/view/script/ScriptTemplateView.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -37,9 +37,7 @@ import org.springframework.context.ApplicationContext; import org.springframework.context.ApplicationContextException; import org.springframework.context.i18n.LocaleContextHolder; import org.springframework.core.io.Resource; -import org.springframework.core.io.buffer.DataBuffer; import org.springframework.http.MediaType; -import org.springframework.http.server.reactive.ServerHttpResponse; import org.springframework.lang.Nullable; import org.springframework.scripting.support.StandardScriptEvalException; import org.springframework.scripting.support.StandardScriptUtils; @@ -301,8 +299,7 @@ public class ScriptTemplateView extends AbstractUrlBasedView { protected Mono renderInternal( Map model, @Nullable MediaType contentType, ServerWebExchange exchange) { - return Mono.defer(() -> { - ServerHttpResponse response = exchange.getResponse(); + return exchange.getResponse().writeWith(Mono.fromCallable(() -> { try { ScriptEngine engine = getEngine(); String url = getUrl(); @@ -338,8 +335,7 @@ public class ScriptTemplateView extends AbstractUrlBasedView { } byte[] bytes = String.valueOf(html).getBytes(StandardCharsets.UTF_8); - DataBuffer buffer = response.bufferFactory().allocateBuffer(bytes.length).write(bytes); - return response.writeWith(Mono.just(buffer)); + return exchange.getResponse().bufferFactory().wrap(bytes); // just wrapping, no allocation } catch (ScriptException ex) { throw new IllegalStateException("Failed to render script template", new StandardScriptEvalException(ex)); @@ -347,7 +343,7 @@ public class ScriptTemplateView extends AbstractUrlBasedView { catch (Exception ex) { throw new IllegalStateException("Failed to render script template", ex); } - }); + })); } protected String getTemplate(String path) throws IOException { diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ZeroDemandResponse.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ZeroDemandResponse.java new file mode 100644 index 00000000000..4e4eee2f6b7 --- /dev/null +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ZeroDemandResponse.java @@ -0,0 +1,129 @@ +/* + * Copyright 2002-2019 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. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.springframework.web.reactive.result.view; + +import java.util.function.Supplier; + +import io.netty.buffer.PooledByteBufAllocator; +import org.reactivestreams.Publisher; +import org.reactivestreams.Subscription; +import reactor.core.publisher.BaseSubscriber; +import reactor.core.publisher.Mono; + +import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.core.io.buffer.DataBufferFactory; +import org.springframework.core.io.buffer.LeakAwareDataBufferFactory; +import org.springframework.core.io.buffer.NettyDataBufferFactory; +import org.springframework.http.HttpHeaders; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseCookie; +import org.springframework.http.server.reactive.ServerHttpResponse; +import org.springframework.util.MultiValueMap; + +/** + * Response that subscribes to the writes source but never posts demand and also + * offers method to then cancel the subscription, and check of leaks in the end. + * + * @author Rossen Stoyanchev + */ +public class ZeroDemandResponse implements ServerHttpResponse { + + private final LeakAwareDataBufferFactory bufferFactory; + + private final ZeroDemandSubscriber writeSubscriber = new ZeroDemandSubscriber(); + + + public ZeroDemandResponse() { + NettyDataBufferFactory delegate = new NettyDataBufferFactory(PooledByteBufAllocator.DEFAULT); + this.bufferFactory = new LeakAwareDataBufferFactory(delegate); + } + + + public void checkForLeaks() { + this.bufferFactory.checkForLeaks(); + } + + public void cancelWrite() { + this.writeSubscriber.cancel(); + } + + + @Override + public DataBufferFactory bufferFactory() { + return this.bufferFactory; + } + + @Override + public Mono writeWith(Publisher body) { + body.subscribe(this.writeSubscriber); + return Mono.never(); + } + + + @Override + public boolean setStatusCode(HttpStatus status) { + throw new UnsupportedOperationException(); + } + + @Override + public HttpStatus getStatusCode() { + throw new UnsupportedOperationException(); + } + + @Override + public MultiValueMap getCookies() { + throw new UnsupportedOperationException(); + } + + @Override + public void addCookie(ResponseCookie cookie) { + throw new UnsupportedOperationException(); + } + + @Override + public void beforeCommit(Supplier> action) { + throw new UnsupportedOperationException(); + } + + @Override + public boolean isCommitted() { + throw new UnsupportedOperationException(); + } + + @Override + public Mono writeAndFlushWith(Publisher> body) { + throw new UnsupportedOperationException(); + } + + @Override + public Mono setComplete() { + throw new UnsupportedOperationException(); + } + + @Override + public HttpHeaders getHeaders() { + throw new UnsupportedOperationException(); + } + + + private static class ZeroDemandSubscriber extends BaseSubscriber { + + @Override + protected void hookOnSubscribe(Subscription subscription) { + // Just subscribe without requesting + } + } +} diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerViewTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerViewTests.java index bc0fb9a2520..831407ce1c3 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerViewTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerViewTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2017 the original author or authors. + * Copyright 2002-2019 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. @@ -31,20 +31,26 @@ import reactor.test.StepVerifier; import org.springframework.context.ApplicationContextException; import org.springframework.context.support.GenericApplicationContext; import org.springframework.core.io.buffer.DataBuffer; +import org.springframework.http.codec.ServerCodecConfigurer; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.web.test.server.MockServerWebExchange; import org.springframework.ui.ExtendedModelMap; import org.springframework.ui.ModelMap; +import org.springframework.web.reactive.result.view.ZeroDemandResponse; +import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.adapter.DefaultServerWebExchange; +import org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver; +import org.springframework.web.server.session.DefaultWebSessionManager; -import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; /** * @author Rossen Stoyanchev */ public class FreeMarkerViewTests { - private static final String TEMPLATE_PATH = "classpath*:org/springframework/web/reactive/view/freemarker/"; + private static final String TEMPLATE_PATH = + "classpath*:org/springframework/web/reactive/view/freemarker/"; private final MockServerWebExchange exchange = @@ -101,7 +107,7 @@ public class FreeMarkerViewTests { } @Test - public void render() throws Exception { + public void render() { FreeMarkerView view = new FreeMarkerView(); view.setConfiguration(this.freeMarkerConfig); view.setUrl("test.ftl"); @@ -116,6 +122,26 @@ public class FreeMarkerViewTests { .verify(); } + @Test // gh-22754 + public void subscribeWithoutDemand() { + ZeroDemandResponse response = new ZeroDemandResponse(); + ServerWebExchange exchange = new DefaultServerWebExchange( + MockServerHttpRequest.get("/path").build(), response, + new DefaultWebSessionManager(), ServerCodecConfigurer.create(), + new AcceptHeaderLocaleContextResolver()); + + FreeMarkerView view = new FreeMarkerView(); + view.setConfiguration(this.freeMarkerConfig); + view.setUrl("test.ftl"); + + ModelMap model = new ExtendedModelMap(); + model.addAttribute("hello", "hi FreeMarker"); + view.render(model, null, exchange).subscribe(); + + response.cancelWrite(); + response.checkForLeaks(); + } + private static String asString(DataBuffer dataBuffer) { ByteBuffer byteBuffer = dataBuffer.asByteBuffer(); diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/script/NashornScriptTemplateTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/script/NashornScriptTemplateTests.java index 22c259219b5..bf23b96ceb8 100644 --- a/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/script/NashornScriptTemplateTests.java +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/view/script/NashornScriptTemplateTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2018 the original author or authors. + * Copyright 2002-2019 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. @@ -25,9 +25,15 @@ import org.springframework.context.annotation.AnnotationConfigApplicationContext import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.http.MediaType; +import org.springframework.http.codec.ServerCodecConfigurer; import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest; import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse; import org.springframework.mock.web.test.server.MockServerWebExchange; +import org.springframework.web.reactive.result.view.ZeroDemandResponse; +import org.springframework.web.server.ServerWebExchange; +import org.springframework.web.server.adapter.DefaultServerWebExchange; +import org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver; +import org.springframework.web.server.session.DefaultWebSessionManager; import static org.junit.Assert.assertEquals; @@ -58,6 +64,25 @@ public class NashornScriptTemplateTests { response.getBodyAsString().block()); } + @Test // gh-22754 + public void subscribeWithoutDemand() throws Exception { + ZeroDemandResponse response = new ZeroDemandResponse(); + ServerWebExchange exchange = new DefaultServerWebExchange( + MockServerHttpRequest.get("/path").build(), response, + new DefaultWebSessionManager(), ServerCodecConfigurer.create(), + new AcceptHeaderLocaleContextResolver()); + + Map model = new HashMap<>(); + model.put("title", "Layout example"); + model.put("body", "This is the body"); + String viewUrl = "org/springframework/web/reactive/result/view/script/nashorn/template.html"; + ScriptTemplateView view = createViewWithUrl(viewUrl, ScriptTemplatingConfiguration.class); + view.render(model, null, exchange).subscribe(); + + response.cancelWrite(); + response.checkForLeaks(); + } + private MockServerHttpResponse render(String viewUrl, Map model, Class configuration) throws Exception {