HTTP Patcher leaks response & stalls segments if no other response listener is supplied
- 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
Assessment
This issue has not been assessed yet.