Skip to content

Commit 1a7f158

Browse files
pavolloffaypavolloffay
authored
Fix servlet exception accessing inputstream before the application (#242)
* Fix servlet exception accessing inputstream before the application Signed-off-by: Pavol Loffay <[email protected]> * More tests Signed-off-by: Pavol Loffay <[email protected]> * Fix name Signed-off-by: Pavol Loffay <[email protected]>
1 parent bb722f2 commit 1a7f158

File tree

4 files changed

+124
-21
lines changed

4 files changed

+124
-21
lines changed

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/BodyCaptureAsyncListener.java

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,24 @@
1818

1919
import io.opentelemetry.api.trace.Span;
2020
import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
21+
import java.io.BufferedReader;
2122
import java.io.PrintWriter;
2223
import java.util.concurrent.atomic.AtomicBoolean;
2324
import javax.servlet.AsyncEvent;
2425
import javax.servlet.AsyncListener;
26+
import javax.servlet.ServletInputStream;
2527
import javax.servlet.ServletOutputStream;
28+
import javax.servlet.ServletRequest;
2629
import javax.servlet.ServletResponse;
30+
import javax.servlet.http.HttpServletRequest;
2731
import javax.servlet.http.HttpServletResponse;
2832
import org.hypertrace.agent.config.Config.AgentConfig;
2933
import org.hypertrace.agent.core.config.HypertraceConfig;
3034
import org.hypertrace.agent.core.instrumentation.HypertraceSemanticAttributes;
3135
import org.hypertrace.agent.core.instrumentation.buffer.BoundedByteArrayOutputStream;
3236
import org.hypertrace.agent.core.instrumentation.buffer.BoundedCharArrayWriter;
37+
import org.hypertrace.agent.core.instrumentation.buffer.ByteBufferSpanPair;
38+
import org.hypertrace.agent.core.instrumentation.buffer.CharBufferSpanPair;
3339
import org.hypertrace.agent.core.instrumentation.utils.ContentTypeUtils;
3440

3541
public class BodyCaptureAsyncListener implements AsyncListener {
@@ -39,30 +45,39 @@ public class BodyCaptureAsyncListener implements AsyncListener {
3945
private final ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> streamContextStore;
4046
private final ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore;
4147

48+
private final ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext;
49+
private final ContextStore<BufferedReader, CharBufferSpanPair> readerContext;
50+
4251
private final AgentConfig agentConfig = HypertraceConfig.get();
4352

4453
public BodyCaptureAsyncListener(
4554
AtomicBoolean responseHandled,
4655
Span span,
4756
ContextStore<ServletOutputStream, BoundedByteArrayOutputStream> streamContextStore,
48-
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore) {
57+
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContextStore,
58+
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext,
59+
ContextStore<BufferedReader, CharBufferSpanPair> readerContext) {
4960
this.responseHandled = responseHandled;
5061
this.span = span;
5162
this.streamContextStore = streamContextStore;
5263
this.writerContextStore = writerContextStore;
64+
this.inputStreamContext = inputStreamContext;
65+
this.readerContext = readerContext;
5366
}
5467

5568
@Override
5669
public void onComplete(AsyncEvent event) {
5770
if (responseHandled.compareAndSet(false, true)) {
58-
captureResponseData(event.getSuppliedResponse());
71+
captureResponseDataAndClearRequestBuffer(
72+
event.getSuppliedResponse(), event.getSuppliedRequest());
5973
}
6074
}
6175

6276
@Override
6377
public void onError(AsyncEvent event) {
6478
if (responseHandled.compareAndSet(false, true)) {
65-
captureResponseData(event.getSuppliedResponse());
79+
captureResponseDataAndClearRequestBuffer(
80+
event.getSuppliedResponse(), event.getSuppliedRequest());
6681
}
6782
}
6883

@@ -72,7 +87,8 @@ public void onTimeout(AsyncEvent event) {}
7287
@Override
7388
public void onStartAsync(AsyncEvent event) {}
7489

75-
private void captureResponseData(ServletResponse servletResponse) {
90+
private void captureResponseDataAndClearRequestBuffer(
91+
ServletResponse servletResponse, ServletRequest servletRequest) {
7692
if (servletResponse instanceof HttpServletResponse) {
7793
HttpServletResponse httpResponse = (HttpServletResponse) servletResponse;
7894

@@ -89,5 +105,14 @@ private void captureResponseData(ServletResponse servletResponse) {
89105
}
90106
}
91107
}
108+
if (servletRequest instanceof HttpServletRequest) {
109+
HttpServletRequest httpRequest = (HttpServletRequest) servletRequest;
110+
111+
// remove request body buffers from context stores, otherwise they might get reused
112+
if (agentConfig.getDataCapture().getHttpBody().getRequest().getValue()
113+
&& ContentTypeUtils.shouldCapture(httpRequest.getContentType())) {
114+
Utils.resetRequestBodyBuffers(inputStreamContext, readerContext, httpRequest);
115+
}
116+
}
92117
}
93118
}

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/main/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/Servlet31NoWrappingInstrumentation.java

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -157,15 +157,11 @@ public static void exit(
157157
ContextStore<PrintWriter, BoundedCharArrayWriter> writerContext =
158158
InstrumentationContext.get(PrintWriter.class, BoundedCharArrayWriter.class);
159159

160-
// remove request body buffers from context stores, otherwise they might get reused
161-
if (agentConfig.getDataCapture().getHttpBody().getRequest().getValue()
162-
&& ContentTypeUtils.shouldCapture(httpRequest.getContentType())) {
163-
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext =
164-
InstrumentationContext.get(ServletInputStream.class, ByteBufferSpanPair.class);
165-
ContextStore<BufferedReader, CharBufferSpanPair> readerContext =
166-
InstrumentationContext.get(BufferedReader.class, CharBufferSpanPair.class);
167-
Utils.resetRequestBodyBuffers(inputStreamContext, readerContext, httpRequest);
168-
}
160+
// request context to clear body buffer
161+
ContextStore<ServletInputStream, ByteBufferSpanPair> inputStreamContext =
162+
InstrumentationContext.get(ServletInputStream.class, ByteBufferSpanPair.class);
163+
ContextStore<BufferedReader, CharBufferSpanPair> readerContext =
164+
InstrumentationContext.get(BufferedReader.class, CharBufferSpanPair.class);
169165

170166
AtomicBoolean responseHandled = new AtomicBoolean(false);
171167
if (request.isAsyncStarted()) {
@@ -174,7 +170,12 @@ public static void exit(
174170
.getAsyncContext()
175171
.addListener(
176172
new BodyCaptureAsyncListener(
177-
responseHandled, currentSpan, outputStreamContext, writerContext));
173+
responseHandled,
174+
currentSpan,
175+
outputStreamContext,
176+
writerContext,
177+
inputStreamContext,
178+
readerContext));
178179
} catch (IllegalStateException e) {
179180
// org.eclipse.jetty.server.Request may throw an exception here if request became
180181
// finished after check above. We just ignore that exception and move on.
@@ -195,6 +196,12 @@ public static void exit(
195196
&& ContentTypeUtils.shouldCapture(httpResponse.getContentType())) {
196197
Utils.captureResponseBody(currentSpan, outputStreamContext, writerContext, httpResponse);
197198
}
199+
200+
// remove request body buffers from context stores, otherwise they might get reused
201+
if (agentConfig.getDataCapture().getHttpBody().getRequest().getValue()
202+
&& ContentTypeUtils.shouldCapture(httpRequest.getContentType())) {
203+
Utils.resetRequestBodyBuffers(inputStreamContext, readerContext, httpRequest);
204+
}
198205
}
199206
}
200207
}

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/Servlet30NoWrappingInstrumentationTest.java

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616

1717
package io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping;
1818

19+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.TestServlets.EchoAsyncResponse_stream;
20+
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.TestServlets.EchoAsyncResponse_writer;
1921
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.TestServlets.EchoStream_arr;
2022
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.TestServlets.EchoStream_arr_offset;
2123
import io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping.TestServlets.EchoStream_readLine_print;
@@ -63,12 +65,14 @@ public static void startServer() throws Exception {
6365
handler.addServlet(TestServlets.EchoWriter_arr.class, "/echo_writer_arr");
6466
handler.addServlet(TestServlets.EchoWriter_arr_offset.class, "/echo_writer_arr_offset");
6567
handler.addServlet(TestServlets.EchoWriter_readLine_write.class, "/echo_writer_readLine_write");
68+
handler.addServlet(TestServlets.EchoWriter_readLines.class, "/echo_writer_readLines");
6669
handler.addServlet(
6770
TestServlets.EchoWriter_readLine_print_str.class, "/echo_writer_readLine_print_str");
6871
handler.addServlet(
6972
TestServlets.EchoWriter_readLine_print_arr.class, "/echo_writer_readLine_print_arr");
7073
handler.addServlet(TestServlets.Forward_to_post.class, "/forward_to_echo");
71-
handler.addServlet(TestServlets.EchoAsyncResponse.class, "/echo_async_response");
74+
handler.addServlet(EchoAsyncResponse_stream.class, "/echo_async_response_stream");
75+
handler.addServlet(EchoAsyncResponse_writer.class, "/echo_async_response_writer");
7276
server.setHandler(handler);
7377
server.start();
7478
serverPort = server.getConnectors()[0].getLocalPort();
@@ -85,8 +89,13 @@ public void forward_to_post() throws Exception {
8589
}
8690

8791
@Test
88-
public void echo_async_response() throws Exception {
89-
postJson(String.format("http://localhost:%d/echo_async_response", serverPort));
92+
public void echo_async_response_stream() throws Exception {
93+
postJson(String.format("http://localhost:%d/echo_async_response_stream", serverPort));
94+
}
95+
96+
@Test
97+
public void echo_async_response_writer() throws Exception {
98+
postJson(String.format("http://localhost:%d/echo_async_response_writer", serverPort));
9099
}
91100

92101
@Test
@@ -134,6 +143,11 @@ public void postJson_writer_readLine_print_str() throws Exception {
134143
postJson(String.format("http://localhost:%d/echo_writer_readLine_print_str", serverPort));
135144
}
136145

146+
@Test
147+
public void postJson_writer_readLines() throws Exception {
148+
postJson(String.format("http://localhost:%d/echo_writer_readLines", serverPort));
149+
}
150+
137151
@Test
138152
public void postJson_writer_readLine_print_arr() throws Exception {
139153
postJson(String.format("http://localhost:%d/echo_writer_readLine_print_arr", serverPort));

‎instrumentation/servlet/servlet-3.0-no-wrapping/src/test/java/io/opentelemetry/javaagent/instrumentation/hypertrace/servlet/v3_0/nowrapping/TestServlets.java

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717
package io.opentelemetry.javaagent.instrumentation.hypertrace.servlet.v3_0.nowrapping;
1818

1919
import java.io.IOException;
20+
import java.util.stream.Stream;
2021
import javax.servlet.AsyncContext;
2122
import javax.servlet.ServletException;
2223
import javax.servlet.http.HttpServlet;
@@ -147,6 +148,20 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
147148
}
148149
}
149150

151+
public static class EchoWriter_readLines extends HttpServlet {
152+
@Override
153+
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
154+
Stream<String> lines = req.getReader().lines();
155+
lines.forEach(s -> {});
156+
157+
resp.setStatus(200);
158+
resp.setContentType("application/json");
159+
resp.setHeader(RESPONSE_HEADER, RESPONSE_HEADER_VALUE);
160+
161+
resp.getWriter().write(RESPONSE_BODY);
162+
}
163+
}
164+
150165
public static class EchoWriter_readLine_print_str extends HttpServlet {
151166
@Override
152167
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
@@ -173,14 +188,56 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
173188
}
174189
}
175190

176-
public static class EchoAsyncResponse extends HttpServlet {
191+
public static class EchoAsyncResponse_stream extends HttpServlet {
177192
@Override
178-
protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException {
179-
while (req.getReader().readLine() != null) {}
193+
protected void service(HttpServletRequest req, HttpServletResponse resp) {
180194

181195
AsyncContext asyncContext = req.startAsync();
182196
asyncContext.start(
183197
() -> {
198+
while (true) {
199+
try {
200+
if (!(req.getInputStream().read() != -1)) break;
201+
} catch (IOException e) {
202+
e.printStackTrace();
203+
}
204+
}
205+
206+
try {
207+
Thread.sleep(100);
208+
} catch (InterruptedException e) {
209+
e.printStackTrace();
210+
}
211+
HttpServletResponse httpServletResponse =
212+
(HttpServletResponse) asyncContext.getResponse();
213+
httpServletResponse.setStatus(200);
214+
httpServletResponse.setContentType("application/json");
215+
httpServletResponse.setHeader(RESPONSE_HEADER, RESPONSE_HEADER_VALUE);
216+
try {
217+
httpServletResponse.getOutputStream().print(RESPONSE_BODY);
218+
} catch (IOException e) {
219+
e.printStackTrace();
220+
}
221+
asyncContext.complete();
222+
});
223+
}
224+
}
225+
226+
public static class EchoAsyncResponse_writer extends HttpServlet {
227+
@Override
228+
protected void service(HttpServletRequest req, HttpServletResponse resp) {
229+
230+
AsyncContext asyncContext = req.startAsync();
231+
asyncContext.start(
232+
() -> {
233+
while (true) {
234+
try {
235+
if (!(req.getReader().read() != -1)) break;
236+
} catch (IOException e) {
237+
e.printStackTrace();
238+
}
239+
}
240+
184241
try {
185242
Thread.sleep(100);
186243
} catch (InterruptedException e) {
@@ -192,7 +249,7 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
192249
httpServletResponse.setContentType("application/json");
193250
httpServletResponse.setHeader(RESPONSE_HEADER, RESPONSE_HEADER_VALUE);
194251
try {
195-
httpServletResponse.getWriter().print(RESPONSE_BODY.toCharArray());
252+
httpServletResponse.getWriter().print(RESPONSE_BODY);
196253
} catch (IOException e) {
197254
e.printStackTrace();
198255
}

0 commit comments

Comments
 (0)