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

[omsmith]

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:

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:

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

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.

[haotianw465]

Thank you for the write up. I noticed the mandatory consumption to close the subsegment if no extra listener added. Leaking memory inside the SDK is definitely a critical issue and the SDK should not force user to consume the response if no user added listener.

The purposed fix looks good to me. We will spend some time testing it and add this fix to the coming release.

[haotianw465]

Fix rolled out with 1.2.0 release.

[Flarna]

I know this is an old issue but I just wondered about this fix done that time to check if there is just one end listener and if yes call res.resume().

Looking into the NodeJs source code it seems that above check will never be true as one end listener is installed by node and a second one by http_p.js.

[willarmiros]

Hi @Flarna,
Yeah, it looks like you are correct. As we discussed in #318, it is impossible to tell whether you are the "only" listener for a given event. However in this case I'm not even sure the fix was necessary, since the quoted part of the Node docs refers to consuming the response, e.g. with a listener to the data event. However we are listening to the end event, so would you agree that this piece is not needed whatsoever? Because there is no risk of data leaking since the end event has necessarily already been emitted, so it will be cleared after the listeners finish with it right?

[omsmith]

The issue is for the response event, not the end event or the data event.

As @Flarna has pointed out, it seems like the fix that landed was incorrect.

[Flarna]

If there is no response listener NodeJs core will consume (dump) the data. If there is a listener it is the user application which has to do this. Either by installing a 'data' event per hand, implicit by piping into some stream or by calling resume().

If the user application is not passing a callback this module will add one and as a result it is responsible to consume the data otherwise connections may hang.

Please note that there are two ways to install a ' response' listener:

• pass a callback as last argument to get/request
• installed it via EventEmitter APIs on the return value of get/request

[willarmiros]

@omsmith @Flarna so it seems like the initially proposed fix is correct. My only question is if we are not the only listener (e.g. req.listenerCount('response') > 1) , should we remove our listener so that if other libraries are making the same check they would see themselves as the only listener? Or would that break the X-Ray SDK's ability to trace keep-alive connections connections?

[Flarna]

It's slightly different (and easier) here compared to the error event case.
If there are more http monitoring tools in the same app all of them will wrap http.get and http.request. As a result the call sequence is User Code => Monitoring Tool 1 => Monitoring Tool 2 => HTTP.

Consider user is not passing a callback and aws-xray-sdk is Monitoring Tool 1. It will add a (dumping) callback and Monitoring Tool 2 thinks users code uses a callback and therefore no need to dump the data - which is correct. If it is the other way around it works the same.

[willarmiros]

I see! Thank you for the explanation. I will include this fix along with that in #318.

[willarmiros]

@Flarna @omsmith I attempted this fix, however whenever I tried to check req.listenerCount('response') it always returned 0. This was in spite of having two callbacks - one added by the client application making the HTTP request and one added by this SDK's HTTP patcher. I even checked the req._events field and it showed that the only events being listened to on the request was error and preFinish, which is only used internally by Node. Am I missing something?

[Flarna]

I did a fast try and http.get("http://localhost/", (res) => {}).listenerCount("response") returns 1 for me.