Skip to content

Commit 48885d2

Browse files
original-brownbearoriginal-brownbear
authored
TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests (#32377)
* TESTS: Fix Buf Leaks in HttpReadWriteHandlerTests * Release all ref counted things that weren't getting properly released * Mannually force channel promise to be completed because mock channel doesn't do it and it prevents one `release` call in `io.netty.channel.ChannelOutboundHandlerAdapter#write` from firing
1 parent 467a60b commit 48885d2

File tree

1 file changed

+125
-69
lines changed

1 file changed

+125
-69
lines changed

‎plugins/transport-nio/src/test/java/org/elasticsearch/http/nio/HttpReadWriteHandlerTests.java‎

Lines changed: 125 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121

2222
import io.netty.buffer.ByteBuf;
2323
import io.netty.buffer.Unpooled;
24+
import io.netty.channel.ChannelPromise;
2425
import io.netty.channel.embedded.EmbeddedChannel;
2526
import io.netty.handler.codec.http.DefaultFullHttpRequest;
2627
import io.netty.handler.codec.http.FullHttpResponse;
@@ -116,38 +117,48 @@ public void testSuccessfulDecodeHttpRequest() throws IOException {
116117

117118
ByteBuf buf = requestEncoder.encode(httpRequest);
118119
int slicePoint = randomInt(buf.writerIndex() - 1);
119-
120120
ByteBuf slicedBuf = buf.retainedSlice(0, slicePoint);
121121
ByteBuf slicedBuf2 = buf.retainedSlice(slicePoint, buf.writerIndex());
122-
handler.consumeReads(toChannelBuffer(slicedBuf));
122+
try {
123+
handler.consumeReads(toChannelBuffer(slicedBuf));
123124

124-
verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class));
125+
verify(transport, times(0)).incomingRequest(any(HttpRequest.class), any(NioHttpChannel.class));
125126

126-
handler.consumeReads(toChannelBuffer(slicedBuf2));
127+
handler.consumeReads(toChannelBuffer(slicedBuf2));
127128

128-
ArgumentCaptor<HttpRequest> requestCaptor = ArgumentCaptor.forClass(HttpRequest.class);
129-
verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class));
129+
ArgumentCaptor<HttpRequest> requestCaptor = ArgumentCaptor.forClass(HttpRequest.class);
130+
verify(transport).incomingRequest(requestCaptor.capture(), any(NioHttpChannel.class));
130131

131-
HttpRequest nioHttpRequest = requestCaptor.getValue();
132-
assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion());
133-
assertEquals(RestRequest.Method.GET, nioHttpRequest.method());
132+
HttpRequest nioHttpRequest = requestCaptor.getValue();
133+
assertEquals(HttpRequest.HttpVersion.HTTP_1_1, nioHttpRequest.protocolVersion());
134+
assertEquals(RestRequest.Method.GET, nioHttpRequest.method());
135+
} finally {
136+
handler.close();
137+
buf.release();
138+
slicedBuf.release();
139+
slicedBuf2.release();
140+
}
134141
}
135142

136143
public void testDecodeHttpRequestError() throws IOException {
137144
String uri = "localhost:9090/" + randomAlphaOfLength(8);
138145
io.netty.handler.codec.http.HttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri);
139146

140147
ByteBuf buf = requestEncoder.encode(httpRequest);
141-
buf.setByte(0, ' ');
142-
buf.setByte(1, ' ');
143-
buf.setByte(2, ' ');
148+
try {
149+
buf.setByte(0, ' ');
150+
buf.setByte(1, ' ');
151+
buf.setByte(2, ' ');
144152

145-
handler.consumeReads(toChannelBuffer(buf));
153+
handler.consumeReads(toChannelBuffer(buf));
146154

147-
ArgumentCaptor<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
148-
verify(transport).incomingRequestError(any(HttpRequest.class), any(NioHttpChannel.class), exceptionCaptor.capture());
155+
ArgumentCaptor<Exception> exceptionCaptor = ArgumentCaptor.forClass(Exception.class);
156+
verify(transport).incomingRequestError(any(HttpRequest.class), any(NioHttpChannel.class), exceptionCaptor.capture());
149157

150-
assertTrue(exceptionCaptor.getValue() instanceof IllegalArgumentException);
158+
assertTrue(exceptionCaptor.getValue() instanceof IllegalArgumentException);
159+
} finally {
160+
buf.release();
161+
}
151162
}
152163

153164
public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() throws IOException {
@@ -157,9 +168,11 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t
157168
HttpUtil.setKeepAlive(httpRequest, false);
158169

159170
ByteBuf buf = requestEncoder.encode(httpRequest);
160-
161-
handler.consumeReads(toChannelBuffer(buf));
162-
171+
try {
172+
handler.consumeReads(toChannelBuffer(buf));
173+
} finally {
174+
buf.release();
175+
}
163176
verify(transport, times(0)).incomingRequestError(any(), any(), any());
164177
verify(transport, times(0)).incomingRequest(any(), any());
165178

@@ -168,13 +181,17 @@ public void testDecodeHttpRequestContentLengthToLongGeneratesOutboundMessage() t
168181

169182
FlushOperation flushOperation = flushOperations.get(0);
170183
FullHttpResponse response = responseDecoder.decode(Unpooled.wrappedBuffer(flushOperation.getBuffersToWrite()));
171-
assertEquals(HttpVersion.HTTP_1_1, response.protocolVersion());
172-
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
173-
174-
flushOperation.getListener().accept(null, null);
175-
// Since we have keep-alive set to false, we should close the channel after the response has been
176-
// flushed
177-
verify(nioHttpChannel).close();
184+
try {
185+
assertEquals(HttpVersion.HTTP_1_1, response.protocolVersion());
186+
assertEquals(HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, response.status());
187+
188+
flushOperation.getListener().accept(null, null);
189+
// Since we have keep-alive set to false, we should close the channel after the response has been
190+
// flushed
191+
verify(nioHttpChannel).close();
192+
} finally {
193+
response.release();
194+
}
178195
}
179196

180197
@SuppressWarnings("unchecked")
@@ -189,21 +206,29 @@ public void testEncodeHttpResponse() throws IOException {
189206
SocketChannelContext context = mock(SocketChannelContext.class);
190207
HttpWriteOperation writeOperation = new HttpWriteOperation(context, httpResponse, mock(BiConsumer.class));
191208
List<FlushOperation> flushOperations = handler.writeToBytes(writeOperation);
192-
193-
FullHttpResponse response = responseDecoder.decode(Unpooled.wrappedBuffer(flushOperations.get(0).getBuffersToWrite()));
194-
195-
assertEquals(HttpResponseStatus.OK, response.status());
196-
assertEquals(HttpVersion.HTTP_1_1, response.protocolVersion());
209+
FlushOperation operation = flushOperations.get(0);
210+
FullHttpResponse response = responseDecoder.decode(Unpooled.wrappedBuffer(operation.getBuffersToWrite()));
211+
((ChannelPromise) operation.getListener()).setSuccess();
212+
try {
213+
assertEquals(HttpResponseStatus.OK, response.status());
214+
assertEquals(HttpVersion.HTTP_1_1, response.protocolVersion());
215+
} finally {
216+
response.release();
217+
}
197218
}
198219

199220
public void testCorsEnabledWithoutAllowOrigins() throws IOException {
200221
// Set up a HTTP transport with only the CORS enabled setting
201222
Settings settings = Settings.builder()
202223
.put(HttpTransportSettings.SETTING_CORS_ENABLED.getKey(), true)
203224
.build();
204-
io.netty.handler.codec.http.HttpResponse response = executeCorsRequest(settings, "remote-host", "request-host");
205-
// inspect response and validate
206-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), nullValue());
225+
FullHttpResponse response = executeCorsRequest(settings, "remote-host", "request-host");
226+
try {
227+
// inspect response and validate
228+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), nullValue());
229+
} finally {
230+
response.release();
231+
}
207232
}
208233

209234
public void testCorsEnabledWithAllowOrigins() throws IOException {
@@ -213,11 +238,15 @@ public void testCorsEnabledWithAllowOrigins() throws IOException {
213238
.put(SETTING_CORS_ENABLED.getKey(), true)
214239
.put(SETTING_CORS_ALLOW_ORIGIN.getKey(), originValue)
215240
.build();
216-
io.netty.handler.codec.http.HttpResponse response = executeCorsRequest(settings, originValue, "request-host");
217-
// inspect response and validate
218-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
219-
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
220-
assertThat(allowedOrigins, is(originValue));
241+
FullHttpResponse response = executeCorsRequest(settings, originValue, "request-host");
242+
try {
243+
// inspect response and validate
244+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
245+
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
246+
assertThat(allowedOrigins, is(originValue));
247+
} finally {
248+
response.release();
249+
}
221250
}
222251

223252
public void testCorsAllowOriginWithSameHost() throws IOException {
@@ -228,29 +257,44 @@ public void testCorsAllowOriginWithSameHost() throws IOException {
228257
.put(SETTING_CORS_ENABLED.getKey(), true)
229258
.build();
230259
FullHttpResponse response = executeCorsRequest(settings, originValue, host);
231-
// inspect response and validate
232-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
233-
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
234-
assertThat(allowedOrigins, is(originValue));
235-
260+
String allowedOrigins;
261+
try {
262+
// inspect response and validate
263+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
264+
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
265+
assertThat(allowedOrigins, is(originValue));
266+
} finally {
267+
response.release();
268+
}
236269
originValue = "http://" + originValue;
237270
response = executeCorsRequest(settings, originValue, host);
238-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
239-
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
240-
assertThat(allowedOrigins, is(originValue));
271+
try {
272+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
273+
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
274+
assertThat(allowedOrigins, is(originValue));
275+
} finally {
276+
response.release();
277+
}
241278

242279
originValue = originValue + ":5555";
243280
host = host + ":5555";
244281
response = executeCorsRequest(settings, originValue, host);
245-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
246-
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
247-
assertThat(allowedOrigins, is(originValue));
248-
282+
try {
283+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
284+
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
285+
assertThat(allowedOrigins, is(originValue));
286+
} finally {
287+
response.release();
288+
}
249289
originValue = originValue.replace("http", "https");
250290
response = executeCorsRequest(settings, originValue, host);
251-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
252-
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
253-
assertThat(allowedOrigins, is(originValue));
291+
try {
292+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
293+
allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
294+
assertThat(allowedOrigins, is(originValue));
295+
} finally {
296+
response.release();
297+
}
254298
}
255299

256300
public void testThatStringLiteralWorksOnMatch() throws IOException {
@@ -261,12 +305,16 @@ public void testThatStringLiteralWorksOnMatch() throws IOException {
261305
.put(SETTING_CORS_ALLOW_METHODS.getKey(), "get, options, post")
262306
.put(SETTING_CORS_ALLOW_CREDENTIALS.getKey(), true)
263307
.build();
264-
io.netty.handler.codec.http.HttpResponse response = executeCorsRequest(settings, originValue, "request-host");
265-
// inspect response and validate
266-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
267-
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
268-
assertThat(allowedOrigins, is(originValue));
269-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_CREDENTIALS), equalTo("true"));
308+
FullHttpResponse response = executeCorsRequest(settings, originValue, "request-host");
309+
try {
310+
// inspect response and validate
311+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
312+
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
313+
assertThat(allowedOrigins, is(originValue));
314+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_CREDENTIALS), equalTo("true"));
315+
} finally {
316+
response.release();
317+
}
270318
}
271319

272320
public void testThatAnyOriginWorks() throws IOException {
@@ -275,12 +323,16 @@ public void testThatAnyOriginWorks() throws IOException {
275323
.put(SETTING_CORS_ENABLED.getKey(), true)
276324
.put(SETTING_CORS_ALLOW_ORIGIN.getKey(), originValue)
277325
.build();
278-
io.netty.handler.codec.http.HttpResponse response = executeCorsRequest(settings, originValue, "request-host");
279-
// inspect response and validate
280-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
281-
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
282-
assertThat(allowedOrigins, is(originValue));
283-
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_CREDENTIALS), nullValue());
326+
FullHttpResponse response = executeCorsRequest(settings, originValue, "request-host");
327+
try {
328+
// inspect response and validate
329+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN), notNullValue());
330+
String allowedOrigins = response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_ORIGIN);
331+
assertThat(allowedOrigins, is(originValue));
332+
assertThat(response.headers().get(HttpHeaderNames.ACCESS_CONTROL_ALLOW_CREDENTIALS), nullValue());
333+
} finally {
334+
response.release();
335+
}
284336
}
285337

286338
private FullHttpResponse executeCorsRequest(final Settings settings, final String originValue, final String host) throws IOException {
@@ -300,8 +352,9 @@ private FullHttpResponse executeCorsRequest(final Settings settings, final Strin
300352

301353
SocketChannelContext context = mock(SocketChannelContext.class);
302354
List<FlushOperation> flushOperations = handler.writeToBytes(handler.createWriteOperation(context, response, (v, e) -> {}));
303-
355+
handler.close();
304356
FlushOperation flushOperation = flushOperations.get(0);
357+
((ChannelPromise) flushOperation.getListener()).setSuccess();
305358
return responseDecoder.decode(Unpooled.wrappedBuffer(flushOperation.getBuffersToWrite()));
306359
}
307360

@@ -314,8 +367,11 @@ private NioHttpRequest prepareHandlerForResponse(HttpReadWriteHandler handler) t
314367

315368
io.netty.handler.codec.http.HttpRequest request = new DefaultFullHttpRequest(version, method, uri);
316369
ByteBuf buf = requestEncoder.encode(request);
317-
318-
handler.consumeReads(toChannelBuffer(buf));
370+
try {
371+
handler.consumeReads(toChannelBuffer(buf));
372+
} finally {
373+
buf.release();
374+
}
319375

320376
ArgumentCaptor<NioHttpRequest> requestCaptor = ArgumentCaptor.forClass(NioHttpRequest.class);
321377
verify(transport, atLeastOnce()).incomingRequest(requestCaptor.capture(), any(HttpChannel.class));

0 commit comments

Comments
 (0)