Skip to content

Commit 57558a4

Browse files
rstoyanchevrstoyanchev
committed
DataBuffer fixes in View implementations
Closes gh-22754
1 parent 6cabb79 commit 57558a4

File tree

6 files changed

+221
-39
lines changed

6 files changed

+221
-39
lines changed

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/method/annotation/AbstractMessageReaderArgumentResolver.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
import org.springframework.core.codec.DecodingException;
3636
import org.springframework.core.codec.Hints;
3737
import org.springframework.core.io.buffer.DataBuffer;
38+
import org.springframework.core.io.buffer.DataBufferUtils;
3839
import org.springframework.http.HttpMethod;
3940
import org.springframework.http.MediaType;
4041
import org.springframework.http.codec.HttpMessageReader;
@@ -200,7 +201,8 @@ protected Mono<Object> readBody(MethodParameter bodyParam, @Nullable MethodParam
200201

201202
HttpMethod method = request.getMethod();
202203
if (contentType == null && method != null && SUPPORTED_METHODS.contains(method)) {
203-
Flux<DataBuffer> body = request.getBody().doOnNext(o -> {
204+
Flux<DataBuffer> body = request.getBody().doOnNext(buffer -> {
205+
DataBufferUtils.release(buffer);
204206
// Body not empty, back to 415..
205207
throw new UnsupportedMediaTypeStatusException(mediaType, this.supportedMediaTypes, elementType);
206208
});

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerView.java

Lines changed: 28 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -32,7 +32,6 @@
3232
import freemarker.template.SimpleHash;
3333
import freemarker.template.Template;
3434
import freemarker.template.Version;
35-
import reactor.core.publisher.Flux;
3635
import reactor.core.publisher.Mono;
3736

3837
import org.springframework.beans.BeansException;
@@ -42,6 +41,7 @@
4241
import org.springframework.context.i18n.LocaleContextHolder;
4342
import org.springframework.core.io.buffer.DataBuffer;
4443
import org.springframework.core.io.buffer.DataBufferUtils;
44+
import org.springframework.core.io.buffer.PooledDataBuffer;
4545
import org.springframework.http.MediaType;
4646
import org.springframework.lang.Nullable;
4747
import org.springframework.util.Assert;
@@ -184,30 +184,34 @@ public boolean checkResourceExists(Locale locale) throws Exception {
184184
protected Mono<Void> renderInternal(Map<String, Object> renderAttributes,
185185
@Nullable MediaType contentType, ServerWebExchange exchange) {
186186

187-
// Expose all standard FreeMarker hash models.
188-
SimpleHash freeMarkerModel = getTemplateModel(renderAttributes, exchange);
187+
return exchange.getResponse().writeWith(Mono
188+
.fromCallable(() -> {
189+
// Expose all standard FreeMarker hash models.
190+
SimpleHash freeMarkerModel = getTemplateModel(renderAttributes, exchange);
189191

190-
if (logger.isDebugEnabled()) {
191-
logger.debug(exchange.getLogPrefix() + "Rendering [" + getUrl() + "]");
192-
}
192+
if (logger.isDebugEnabled()) {
193+
logger.debug(exchange.getLogPrefix() + "Rendering [" + getUrl() + "]");
194+
}
193195

194-
Locale locale = LocaleContextHolder.getLocale(exchange.getLocaleContext());
195-
DataBuffer dataBuffer = exchange.getResponse().bufferFactory().allocateBuffer();
196-
try {
197-
Charset charset = getCharset(contentType);
198-
Writer writer = new OutputStreamWriter(dataBuffer.asOutputStream(), charset);
199-
getTemplate(locale).process(freeMarkerModel, writer);
200-
}
201-
catch (IOException ex) {
202-
DataBufferUtils.release(dataBuffer);
203-
String message = "Could not load FreeMarker template for URL [" + getUrl() + "]";
204-
return Mono.error(new IllegalStateException(message, ex));
205-
}
206-
catch (Throwable ex) {
207-
DataBufferUtils.release(dataBuffer);
208-
return Mono.error(ex);
209-
}
210-
return exchange.getResponse().writeWith(Flux.just(dataBuffer));
196+
Locale locale = LocaleContextHolder.getLocale(exchange.getLocaleContext());
197+
DataBuffer dataBuffer = exchange.getResponse().bufferFactory().allocateBuffer();
198+
try {
199+
Charset charset = getCharset(contentType);
200+
Writer writer = new OutputStreamWriter(dataBuffer.asOutputStream(), charset);
201+
getTemplate(locale).process(freeMarkerModel, writer);
202+
return dataBuffer;
203+
}
204+
catch (IOException ex) {
205+
DataBufferUtils.release(dataBuffer);
206+
String message = "Could not load FreeMarker template for URL [" + getUrl() + "]";
207+
throw new IllegalStateException(message, ex);
208+
}
209+
catch (Throwable ex) {
210+
DataBufferUtils.release(dataBuffer);
211+
throw ex;
212+
}
213+
})
214+
.doOnDiscard(PooledDataBuffer.class, DataBufferUtils::release));
211215
}
212216

213217
private Charset getCharset(@Nullable MediaType mediaType) {

‎spring-webflux/src/main/java/org/springframework/web/reactive/result/view/script/ScriptTemplateView.java

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2018 the original author or authors.
2+
* Copyright 2002-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -37,9 +37,7 @@
3737
import org.springframework.context.ApplicationContextException;
3838
import org.springframework.context.i18n.LocaleContextHolder;
3939
import org.springframework.core.io.Resource;
40-
import org.springframework.core.io.buffer.DataBuffer;
4140
import org.springframework.http.MediaType;
42-
import org.springframework.http.server.reactive.ServerHttpResponse;
4341
import org.springframework.lang.Nullable;
4442
import org.springframework.scripting.support.StandardScriptEvalException;
4543
import org.springframework.scripting.support.StandardScriptUtils;
@@ -301,8 +299,7 @@ public boolean checkResourceExists(Locale locale) throws Exception {
301299
protected Mono<Void> renderInternal(
302300
Map<String, Object> model, @Nullable MediaType contentType, ServerWebExchange exchange) {
303301

304-
return Mono.defer(() -> {
305-
ServerHttpResponse response = exchange.getResponse();
302+
return exchange.getResponse().writeWith(Mono.fromCallable(() -> {
306303
try {
307304
ScriptEngine engine = getEngine();
308305
String url = getUrl();
@@ -338,16 +335,15 @@ else if (this.renderObject != null) {
338335
}
339336

340337
byte[] bytes = String.valueOf(html).getBytes(StandardCharsets.UTF_8);
341-
DataBuffer buffer = response.bufferFactory().allocateBuffer(bytes.length).write(bytes);
342-
return response.writeWith(Mono.just(buffer));
338+
return exchange.getResponse().bufferFactory().wrap(bytes); // just wrapping, no allocation
343339
}
344340
catch (ScriptException ex) {
345341
throw new IllegalStateException("Failed to render script template", new StandardScriptEvalException(ex));
346342
}
347343
catch (Exception ex) {
348344
throw new IllegalStateException("Failed to render script template", ex);
349345
}
350-
});
346+
}));
351347
}
352348

353349
protected String getTemplate(String path) throws IOException {

‎spring-webflux/src/test/java/org/springframework/web/reactive/result/view/ZeroDemandResponse.java

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
/*
2+
* Copyright 2002-2019 the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
package org.springframework.web.reactive.result.view;
17+
18+
import java.util.function.Supplier;
19+
20+
import io.netty.buffer.PooledByteBufAllocator;
21+
import org.reactivestreams.Publisher;
22+
import org.reactivestreams.Subscription;
23+
import reactor.core.publisher.BaseSubscriber;
24+
import reactor.core.publisher.Mono;
25+
26+
import org.springframework.core.io.buffer.DataBuffer;
27+
import org.springframework.core.io.buffer.DataBufferFactory;
28+
import org.springframework.core.io.buffer.LeakAwareDataBufferFactory;
29+
import org.springframework.core.io.buffer.NettyDataBufferFactory;
30+
import org.springframework.http.HttpHeaders;
31+
import org.springframework.http.HttpStatus;
32+
import org.springframework.http.ResponseCookie;
33+
import org.springframework.http.server.reactive.ServerHttpResponse;
34+
import org.springframework.util.MultiValueMap;
35+
36+
/**
37+
* Response that subscribes to the writes source but never posts demand and also
38+
* offers method to then cancel the subscription, and check of leaks in the end.
39+
*
40+
* @author Rossen Stoyanchev
41+
*/
42+
public class ZeroDemandResponse implements ServerHttpResponse {
43+
44+
private final LeakAwareDataBufferFactory bufferFactory;
45+
46+
private final ZeroDemandSubscriber writeSubscriber = new ZeroDemandSubscriber();
47+
48+
49+
public ZeroDemandResponse() {
50+
NettyDataBufferFactory delegate = new NettyDataBufferFactory(PooledByteBufAllocator.DEFAULT);
51+
this.bufferFactory = new LeakAwareDataBufferFactory(delegate);
52+
}
53+
54+
55+
public void checkForLeaks() {
56+
this.bufferFactory.checkForLeaks();
57+
}
58+
59+
public void cancelWrite() {
60+
this.writeSubscriber.cancel();
61+
}
62+
63+
64+
@Override
65+
public DataBufferFactory bufferFactory() {
66+
return this.bufferFactory;
67+
}
68+
69+
@Override
70+
public Mono<Void> writeWith(Publisher<? extends DataBuffer> body) {
71+
body.subscribe(this.writeSubscriber);
72+
return Mono.never();
73+
}
74+
75+
76+
@Override
77+
public boolean setStatusCode(HttpStatus status) {
78+
throw new UnsupportedOperationException();
79+
}
80+
81+
@Override
82+
public HttpStatus getStatusCode() {
83+
throw new UnsupportedOperationException();
84+
}
85+
86+
@Override
87+
public MultiValueMap<String, ResponseCookie> getCookies() {
88+
throw new UnsupportedOperationException();
89+
}
90+
91+
@Override
92+
public void addCookie(ResponseCookie cookie) {
93+
throw new UnsupportedOperationException();
94+
}
95+
96+
@Override
97+
public void beforeCommit(Supplier<? extends Mono<Void>> action) {
98+
throw new UnsupportedOperationException();
99+
}
100+
101+
@Override
102+
public boolean isCommitted() {
103+
throw new UnsupportedOperationException();
104+
}
105+
106+
@Override
107+
public Mono<Void> writeAndFlushWith(Publisher<? extends Publisher<? extends DataBuffer>> body) {
108+
throw new UnsupportedOperationException();
109+
}
110+
111+
@Override
112+
public Mono<Void> setComplete() {
113+
throw new UnsupportedOperationException();
114+
}
115+
116+
@Override
117+
public HttpHeaders getHeaders() {
118+
throw new UnsupportedOperationException();
119+
}
120+
121+
122+
private static class ZeroDemandSubscriber extends BaseSubscriber<DataBuffer> {
123+
124+
@Override
125+
protected void hookOnSubscribe(Subscription subscription) {
126+
// Just subscribe without requesting
127+
}
128+
}
129+
}

‎spring-webflux/src/test/java/org/springframework/web/reactive/result/view/freemarker/FreeMarkerViewTests.java

Lines changed: 31 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2017 the original author or authors.
2+
* Copyright 2002-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -31,20 +31,26 @@
3131
import org.springframework.context.ApplicationContextException;
3232
import org.springframework.context.support.GenericApplicationContext;
3333
import org.springframework.core.io.buffer.DataBuffer;
34+
import org.springframework.http.codec.ServerCodecConfigurer;
3435
import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest;
3536
import org.springframework.mock.web.test.server.MockServerWebExchange;
3637
import org.springframework.ui.ExtendedModelMap;
3738
import org.springframework.ui.ModelMap;
39+
import org.springframework.web.reactive.result.view.ZeroDemandResponse;
40+
import org.springframework.web.server.ServerWebExchange;
41+
import org.springframework.web.server.adapter.DefaultServerWebExchange;
42+
import org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver;
43+
import org.springframework.web.server.session.DefaultWebSessionManager;
3844

39-
import static org.junit.Assert.assertEquals;
40-
import static org.junit.Assert.assertTrue;
45+
import static org.junit.Assert.*;
4146

4247
/**
4348
* @author Rossen Stoyanchev
4449
*/
4550
public class FreeMarkerViewTests {
4651

47-
private static final String TEMPLATE_PATH = "classpath*:org/springframework/web/reactive/view/freemarker/";
52+
private static final String TEMPLATE_PATH =
53+
"classpath*:org/springframework/web/reactive/view/freemarker/";
4854

4955

5056
private final MockServerWebExchange exchange =
@@ -101,7 +107,7 @@ public void checkResourceExists() throws Exception {
101107
}
102108

103109
@Test
104-
public void render() throws Exception {
110+
public void render() {
105111
FreeMarkerView view = new FreeMarkerView();
106112
view.setConfiguration(this.freeMarkerConfig);
107113
view.setUrl("test.ftl");
@@ -116,6 +122,26 @@ public void render() throws Exception {
116122
.verify();
117123
}
118124

125+
@Test // gh-22754
126+
public void subscribeWithoutDemand() {
127+
ZeroDemandResponse response = new ZeroDemandResponse();
128+
ServerWebExchange exchange = new DefaultServerWebExchange(
129+
MockServerHttpRequest.get("/path").build(), response,
130+
new DefaultWebSessionManager(), ServerCodecConfigurer.create(),
131+
new AcceptHeaderLocaleContextResolver());
132+
133+
FreeMarkerView view = new FreeMarkerView();
134+
view.setConfiguration(this.freeMarkerConfig);
135+
view.setUrl("test.ftl");
136+
137+
ModelMap model = new ExtendedModelMap();
138+
model.addAttribute("hello", "hi FreeMarker");
139+
view.render(model, null, exchange).subscribe();
140+
141+
response.cancelWrite();
142+
response.checkForLeaks();
143+
}
144+
119145

120146
private static String asString(DataBuffer dataBuffer) {
121147
ByteBuffer byteBuffer = dataBuffer.asByteBuffer();

‎spring-webflux/src/test/java/org/springframework/web/reactive/result/view/script/NashornScriptTemplateTests.java

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2002-2018 the original author or authors.
2+
* Copyright 2002-2019 the original author or authors.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -25,9 +25,15 @@
2525
import org.springframework.context.annotation.Bean;
2626
import org.springframework.context.annotation.Configuration;
2727
import org.springframework.http.MediaType;
28+
import org.springframework.http.codec.ServerCodecConfigurer;
2829
import org.springframework.mock.http.server.reactive.test.MockServerHttpRequest;
2930
import org.springframework.mock.http.server.reactive.test.MockServerHttpResponse;
3031
import org.springframework.mock.web.test.server.MockServerWebExchange;
32+
import org.springframework.web.reactive.result.view.ZeroDemandResponse;
33+
import org.springframework.web.server.ServerWebExchange;
34+
import org.springframework.web.server.adapter.DefaultServerWebExchange;
35+
import org.springframework.web.server.i18n.AcceptHeaderLocaleContextResolver;
36+
import org.springframework.web.server.session.DefaultWebSessionManager;
3137

3238
import static org.junit.Assert.assertEquals;
3339

@@ -58,6 +64,25 @@ public void renderTemplateWithUrl() throws Exception {
5864
response.getBodyAsString().block());
5965
}
6066

67+
@Test // gh-22754
68+
public void subscribeWithoutDemand() throws Exception {
69+
ZeroDemandResponse response = new ZeroDemandResponse();
70+
ServerWebExchange exchange = new DefaultServerWebExchange(
71+
MockServerHttpRequest.get("/path").build(), response,
72+
new DefaultWebSessionManager(), ServerCodecConfigurer.create(),
73+
new AcceptHeaderLocaleContextResolver());
74+
75+
Map<String, Object> model = new HashMap<>();
76+
model.put("title", "Layout example");
77+
model.put("body", "This is the body");
78+
String viewUrl = "org/springframework/web/reactive/result/view/script/nashorn/template.html";
79+
ScriptTemplateView view = createViewWithUrl(viewUrl, ScriptTemplatingConfiguration.class);
80+
view.render(model, null, exchange).subscribe();
81+
82+
response.cancelWrite();
83+
response.checkForLeaks();
84+
}
85+
6186
private MockServerHttpResponse render(String viewUrl, Map<String, Object> model,
6287
Class<?> configuration) throws Exception {
6388

0 commit comments

Comments
 (0)