Fix for slab buffer retention, leading to large memory consumption #1143

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
6 participants
@jmatthewsr-ms
  • Admittedly undertested. This issues also affects early revs of socket.io. The 'ws' library also exhibits this behavior and a pull request has been created for that lib.

See: https://github.com/jmatthewsr-ms/node-slab-memory-issues

@jmatthewsr-ms

This comment has been minimized.

Show comment
Hide comment
@jmatthewsr-ms

jmatthewsr-ms Jan 25, 2013

Putting this on hold as I believe that a node core fix is the most appropriate. Buffer can also retain an 8k slab on it's own, ideally node core should emit a buffer that is not backed by any slab.

Putting this on hold as I believe that a node core fix is the most appropriate. Buffer can also retain an 8k slab on it's own, ideally node core should emit a buffer that is not backed by any slab.

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jan 26, 2013

Contributor

Agreed

Contributor

rauchg commented Jan 26, 2013

Agreed

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Jan 26, 2013

Contributor

@jmatthewsr-ms is there a bug in the joyent/node that we can follow about this?

Contributor

3rd-Eden commented Jan 26, 2013

@jmatthewsr-ms is there a bug in the joyent/node that we can follow about this?

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Jan 26, 2013

Contributor

Found it: nodejs/node-v0.x-archive#4660

Thanks for the awesome research btw, this is definitely a bit issue we are facing in socket.io and probably engine.io as well.

Contributor

3rd-Eden commented Jan 26, 2013

Found it: nodejs/node-v0.x-archive#4660

Thanks for the awesome research btw, this is definitely a bit issue we are facing in socket.io and probably engine.io as well.

@3rd-Eden

This comment has been minimized.

Show comment
Hide comment
@3rd-Eden

3rd-Eden Feb 20, 2013

Contributor

As this is not being addressed by the Node core team, it seems to me that we need to address this our selfs. What do you think @guille?

Contributor

3rd-Eden commented Feb 20, 2013

As this is not being addressed by the Node core team, it seems to me that we need to address this our selfs. What do you think @guille?

@jpallen

This comment has been minimized.

Show comment
Hide comment
@jpallen

jpallen Feb 27, 2013

I've just run into this issue myself in production and opened up another issue and pull request (#1177 #1178) before finding this issue. If my vote counts for anything, I like the solution here better than my own. It would be nice if this could make it into 0.9, even if you decide on a better solution for 1.0, since we (and many other people) are using Socket.io in production right now and need this fix.

Edit: You can see from the graphs in #1177 that this is quite a serious memory leak in a production server doing large or many requests over HTTPS.

jpallen commented Feb 27, 2013

I've just run into this issue myself in production and opened up another issue and pull request (#1177 #1178) before finding this issue. If my vote counts for anything, I like the solution here better than my own. It would be nice if this could make it into 0.9, even if you decide on a better solution for 1.0, since we (and many other people) are using Socket.io in production right now and need this fix.

Edit: You can see from the graphs in #1177 that this is quite a serious memory leak in a production server doing large or many requests over HTTPS.

@jmatthewsr-ms

This comment has been minimized.

Show comment
Hide comment
@jmatthewsr-ms

jmatthewsr-ms Apr 9, 2013

Note that this is being worked on in node core: nodejs/node-v0.x-archive#4964.

The buffer copy in this fix may not be required once node core buffer is fixed. The fix here shouldn't hurt performance if left in, but worth checking once node core is updated.

Note that this is being worked on in node core: nodejs/node-v0.x-archive#4964.

The buffer copy in this fix may not be required once node core buffer is fixed. The fix here shouldn't hurt performance if left in, but worth checking once node core is updated.

@timewarrior

This comment has been minimized.

Show comment
Hide comment
@timewarrior

timewarrior Jun 7, 2013

Hi Everyone, thanks for being on top of this. I had a quick question. I understand that a fix is being worked on node for this and meanwhile japplen's patch has made it into 6e25c80 . We are trying to launch something based on socket.io soon. Can someone please confirm that 6e25c80 fixes the patch being mentioned here.
Thanks

Hi Everyone, thanks for being on top of this. I had a quick question. I understand that a fix is being worked on node for this and meanwhile japplen's patch has made it into 6e25c80 . We are trying to launch something based on socket.io soon. Can someone please confirm that 6e25c80 fixes the patch being mentioned here.
Thanks

@jpallen

This comment has been minimized.

Show comment
Hide comment
@jpallen

jpallen Jun 10, 2013

Yes, the patch here has the same effect as 6e25c80. The purpose of both is to dereference the head buffer which is passed to the upgrade event handler since this was the cause of the memory leak.

jpallen commented Jun 10, 2013

Yes, the patch here has the same effect as 6e25c80. The purpose of both is to dereference the head buffer which is passed to the upgrade event handler since this was the cause of the memory leak.

@timewarrior

This comment has been minimized.

Show comment
Hide comment
@timewarrior

timewarrior Jun 10, 2013

Thx jpallen :)

Thx jpallen :)

@fjakobs fjakobs referenced this pull request in socketio/engine.io Jul 17, 2014

Closed

port https memory leak fix from socket.io #268

@rauchg

This comment has been minimized.

Show comment
Hide comment
@rauchg

rauchg Jul 18, 2014

Contributor

Fixing on engine.io (socketio/engine.io#268)

Contributor

rauchg commented Jul 18, 2014

Fixing on engine.io (socketio/engine.io#268)

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