aws / aws/aws-xray-sdk-node

HTTP Patcher leaks response & stalls segments if no other response listener is supplied

Open
#18 11 comments 0 reactions 1 assignee Claimed by @haotianw465 View on GitHub
bug
Dominant language
JavaScript
Stars
280
Forks
157
PR merge metrics
No merged PRs in 30d

Description

Ran into this while testing the examples I was providing in #11. Not sure how much real-world impact this has, but the http patcher adds a _response_ listener to outgoing http requests, and then sets up a listener for `res.on('end', ...)` in order to close the subsegment. The problem is, once you do add a _response_ listener, then the response _must_ be consumed.

From the [http documentation](https://nodejs.org/api/http.html#http_class_http_clientrequest):
> If no 'response' handler is added, then the response will be entirely discarded. However, if a 'response' event handler is added, then the data from the response object must be consumed, either by calling response.read() whenever there is a 'readable' event, or by adding a 'data' handler, or by calling the .resume() method. Until the data is consumed, the 'end' event will not fire. Also, until the data is read it will consume memory that can eventually lead to a 'process out of memory' error.

Currently if the developer didn't add a listener of their own, the response is never consumed, meaning the
response data is leaked, _and_ since `end` is never fired, the subsegment will never close meaning it and its parent (sub)segment will never be flushed and sent to the daemon.

Reason I'm not sure how much real-world impact this would have, is I'm not sure how often you would simply not care about the response, but I'm sure there's some sort of fire-and-forget systems out there...?

The following _should_ fix it, but I've not developed any tests for it. It consumes the body if a callback wasn't provided, and if x-ray is the only response listener. Meaning neither of the two scenarios below occurred:

```js
http.request('http://example.com', res => res.resume()).end();
http.request('http://example.com').on('response', res => res.resume()).end();
```

```diff
diff --git a/packages/core/lib/patchers/http_p.js b/packages/core/lib/patchers/http_p.js
index 5c0cb68..f5311ef 100644
--- a/packages/core/lib/patchers/http_p.js
+++ b/packages/core/lib/patchers/http_p.js
@@ -137,6 +137,8 @@ function enableCapture(module, downstreamXRayEnabled) {
} else {
callback(res);
}
+ } else if (req.listenerCount('response') === 1) {
+ res.resume();
}
}).on('error', errorCapturer);
```

Of course, if the developer _does_ add their own listener and neglects to consume the response, then you still run into the segment never closing, but that's a different issue, with developer error mixed in.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.