search

spdy-http2 / node-spdy

SPDY server on Node.js
2.81k stars 196 forks source link

Fix memory leak caused by misfiring callback #287

Closed anandsuresh closed 7 years ago

anandsuresh commented 7 years ago

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.

indutny commented 7 years ago

@anandsuresh the tests doesn't seem to fail without changes to lib/ folder... I clearly see the issue here, but looks like the tests don't reproduce it.

indutny commented 7 years ago

I've managed to create a test for it, but unfortunately this one starts fail:

1) SPDY Server h2 ssl mode should process POST request: Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

Looking into this.

anandsuresh commented 7 years ago

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 wrote:

I've managed to create a test for it, but unfortunately this one starts fail:

1) SPDY Server h2 ssl mode should process POST request: Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

Looking into this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/indutny/node-spdy/pull/287#issuecomment-253217416, or mute the thread https://github.com/notifications/unsubscribe-auth/AA-tuUu4tD7TmJpul01oLg67fevM9Ho7ks5qzORrgaJpZM4KS_Z2 .

indutny commented 7 years ago

Landed with fixes in 6a2e14b. Turns out we should have used setImmediate there.

indutny commented 7 years ago

Thank you!

indutny commented 7 years ago

See https://github.com/nodejs/node/pull/9066

anandsuresh commented 7 years ago

@indutny That is pretty damn sweet!!! Thanks for the merge/publish.