Fix memory leak caused by misfiring callback #287
Conversation
Streams can be ended by calling the `.close()` or `.shutdown()` methods on the Handle instances. Internally, both call the `.end()` method to end the stream. However, only the `.close()` method takes a callback that eventually frees the HttpParser instance allocated to the stream socket. In the event that the `.shutdown()` method is called before calling the `.close()` method, the second call to `.end()` never fires the callback, resulting in the leaking of HttpParser instances along with all its references. This commit fixes the issue by directly executing the callback provided to the `.close()` method in the event the `.shutdown()` method was called before the `.close()` method of the Handle instance.
| ok: 'yes' | ||
| }); | ||
| res.end('response'); | ||
| setTimeout(function() { |
There was a problem hiding this comment.
Not my proudest moment, but it was either this or introducing a nested process.nextTick() in handle.js, which was uglier. The issue is that by the time the test gets around to reading the incoming data from the client, the response has ended, and the call to stream.end() ends up terminating the whole stream before the end event fires on the request object.
Also, it was only the HTTP/2 tests that were failing. The SPDY tests were working fine.
| if (stream._spdyState.isServer && stream._writableState.ending) { | ||
| process.nextTick(callback); | ||
| } else { | ||
| stream.end(callback); |
| @@ -91,4 +91,3 @@ exports.everyConfig = function everyConfig(body) { | |||
| }); | |||
| }); | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Superfluous change, will fix it myself.
|
@anandsuresh the tests doesn't seem to fail without changes to |
|
I've managed to create a test for it, but unfortunately this one starts fail:
Looking into this. |
|
That's the error I see at my end when I ran the tests. Running node v4.5.0. On Wed, Oct 12, 2016, 6:45 AM Fedor Indutny notifications@github.com
|
|
Landed with fixes in 6a2e14b. Turns out we should have used |
|
Thank you! |
|
See nodejs/node#9066 |
|
@indutny That is pretty damn sweet!!! Thanks for the merge/publish. |
Streams can be ended by calling the
.close()or.shutdown()methodson the Handle instances. Internally, both call the
.end()method toend the stream. However, only the
.close()method takes a callbackthat eventually frees the HttpParser instance allocated to the stream
socket.
In the event that the
.shutdown()method is called before calling the.close()method, the second call to.end()never fires the callback,resulting in the leaking of HttpParser instances along with all its
references.
This commit fixes the issue by directly executing the callback provided
to the
.close()method in the event the.shutdown()method wascalled before the
.close()method of the Handle instance.