Release memory used by PerMessageDeflate extension

[darrachequesne]

Hi! It seemed the memory allocated by perMessageDeflate is not properly freed, thus causing memory leaks when this feature is enabled (as reported here #497)

As documented here and implemented here, calling .close() on DeflateRaw and InflateRaw objects should (at least partially) fix these leaks.

Related: socketio/socket.io#2251

(please tell me this is it)

[darrachequesne]

(just to be sure ☀️) ping @3rd-Eden @nkzawa

[nkzawa]

👍 awesome

But I'm not sure if this is the root cause, since zlib objects should be released with socket instances when no reference exists.
Anyway explicit cleaning up would be nice.

[darrachequesne]

@nkzawa hi! that's what I thought too, but running the following gist seems to show a different behaviour

• with perMessageDeflate: false, RSS tends to stabilize around 70 Mo
• with perMessageDeflate: true (without this commit), RSS tends to stabilize around 180 Mo
• with perMessageDeflate: true (with this commit), RSS tends to stabilize around 100 Mo

Meanwhile, the HeapSize goes up and down (which is normal, as GC runs).

Are you able to reproduce my results? Or is there a problem with my test?

I mean, the extra memory is normal, as it comes as a cost for compression (as said here). But as said here (that's a lot of links 😄), memory has to be deallocated by calling deflateEnd() (or inflateEnd()), which is what .close() does here.

But it may not be the root cause (and that's where I need your help).

Note: node -v returns v0.12.7

[nkzawa]

Hi, sorry for the delay. I could reproduce similar results with your scripts, but maybe this means it's not leak? Though I have no idea about the difference of RSS.

[darrachequesne]

You're right, the term 'leak' was probably wrongly used here, please excuse me.

I found this nodejs/node-v0.x-archive#4172. Could this be related and explain the difference in RSS? (as we are now explicitly calling .close())

Source: https://github.com/nodejs/node/blob/master/src/node_zlib.cc#L97

[rauchg]

@3rd-Eden can you merge please?

[3rd-Eden]

PR will be adressed this week. Sorry for the hold up.

[darrachequesne]

Great, thanks!

[yuyi]

Maybe this issue is still at #804 .