This repository has been archived by the owner. It is now read-only.

Fixes #3179 http.js leak while using http.get/request #3181

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@vvo

vvo commented Apr 26, 2012

Made a small changes that could means big change for anyone using http.get/request (like @mikeal request)

We were previously using self.on('response') on ClientRequest, meaning that any ClientRequest would stay in memory even when response is emitted.

So I changed that with .once since we should not get more than one reponse for a particular ClientRequest right ?

I hope I'm not mistaking. Tell me.

In #3179 I have attached a test file and some insights on memory usage from various node versions.

@stefounet

This comment has been minimized.

Show comment
Hide comment
@stefounet

stefounet Apr 27, 2012

Tested on v0.6.15 : no more memory leak and tests ok (make test).

stefounet commented Apr 27, 2012

Tested on v0.6.15 : no more memory leak and tests ok (make test).

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs May 2, 2012

.once is the correct behavior here, since response can only happen once.

@vvo Can you please sign the CLA so that I can take your patch? Thanks.

isaacs commented May 2, 2012

.once is the correct behavior here, since response can only happen once.

@vvo Can you please sign the CLA so that I can take your patch? Thanks.

@vvo

This comment has been minimized.

Show comment
Hide comment
@vvo

vvo May 2, 2012

Done,

I hope that the merge remote upstream did not break the pull request, I'm discovering complex git situations :)

vvo commented May 2, 2012

Done,

I hope that the merge remote upstream did not break the pull request, I'm discovering complex git situations :)

@mikeal

This comment has been minimized.

Show comment
Hide comment
@mikeal

mikeal May 2, 2012

Member

this is good code. more of these please :)

Member

mikeal commented May 2, 2012

this is good code. more of these please :)

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs May 5, 2012

Landed on v0.6 and master.

isaacs commented May 5, 2012

Landed on v0.6 and master.

@isaacs isaacs closed this May 5, 2012

@isaacs

This comment has been minimized.

Show comment
Hide comment
@isaacs

isaacs May 5, 2012

Also, I echo @mikeal's sentiment. Always nice to squash a bug with a 2-char fix :)

isaacs commented May 5, 2012

Also, I echo @mikeal's sentiment. Always nice to squash a bug with a 2-char fix :)

@c4milo

This comment has been minimized.

Show comment
Hide comment
@c4milo

c4milo May 5, 2012

nice work @vvo

c4milo commented May 5, 2012

nice work @vvo

@vvo

This comment has been minimized.

Show comment
Hide comment
@vvo

vvo May 6, 2012

Thanks all, I was thrilled to commit to node.js and will definitely look for more bugs to fix!

vvo commented May 6, 2012

Thanks all, I was thrilled to commit to node.js and will definitely look for more bugs to fix!

@longtian

This comment has been minimized.

Show comment
Hide comment
@longtian

longtian commented on e138f76 May 28, 2015

amazing

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.