Streams v2 problem with SSL

[jerem]

It seems streams 2 with SSL doesn't work properly. Indeed, as soon as you read something on the stream it internally keeps reading the whole content of the stream. (Tested on both 0.9.7 and master branch as of today)

Have a look at the following code and see how the memory and the buffer length keep increasing:

require('https').get('https://www.kernel.org/pub/linux/kernel/v3.0/linux-3.7.4.tar.bz2', function(response) {
    response.read(1);
    setInterval(function() {
      console.log('RSS', process.memoryUsage().rss, 'Buffer Length:', response._readableState.length);
    }, 1000);
});

Which outputs something like this on my machine:

RSS 21098496 Buffer Length: 573440
RSS 21839872 Buffer Length: 884736
RSS 22654976 Buffer Length: 1245184
RSS 23351296 Buffer Length: 1589248
RSS 24072192 Buffer Length: 1949696
RSS 25018368 Buffer Length: 2310144
...
RSS 38875136 Buffer Length: 11583488
RSS 39202816 Buffer Length: 11911168
RSS 39596032 Buffer Length: 12304384
RSS 39956480 Buffer Length: 12648448

The exact same code without SSL works properly:

require('http').get('http://www.kernel.org/pub/linux/kernel/v3.0/linux-3.7.4.tar.bz2', function(response) {
    response.read(1);
    setInterval(function() {
      console.log('RSS', process.memoryUsage().rss, 'Buffer Length:', response._readableState.length);
    }, 1000);
});

Output:

RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063
RSS 17129472 Buffer Length: 17063

[TooTallNate]

/cc @isaacs

[isaacs]

Wow, CryptoStream never got streams2-ified. That's a pretty huge oversight on my part. Nice catch.

Short term, this will stop the leak:

diff --git a/lib/http.js b/lib/http.js
index ae6976a..f52624b 100644
--- a/lib/http.js
+++ b/lib/http.js
@@ -36,13 +36,21 @@ if (process.env.NODE_DEBUG && /http/.test(process.env.NODE_DEBUG)) {
 }

 function readStart(socket) {
-  if (!socket || !socket._handle || !socket._handle.readStart) return;
-  socket._handle.readStart();
+  if (!socket)
+    return;
+  if (socket._handle && socket._handle.readStart)
+    socket._handle.readStart();
+  else if (socket.resume)
+    socket.resume();
 }

 function readStop(socket) {
-  if (!socket || !socket._handle || !socket._handle.readStop) return;
-  socket._handle.readStop();
+  if (!socket)
+    return;
+  if (socket._handle && socket._handle.readStop)
+    socket._handle.readStop();
+  else if (socket.pause)
+    socket.pause();
 }

 // Only called in the slow case where slow means

but more properly, CryptoStream needs to be refactored.

[indutny]

I can probably look into it. (CryptoStream 2.0)

[isaacs]

@indutny Your help would be much appreciated. Also, it'd be a good thing to get your head around streams2 so we have another person who can debug issues.

[indutny]

Done in d59beb9

[jerem]

Gentlemen, my test case posted in the issue is still not working properly. 😱

[indutny]

Confirming, the problem is so obvious that it makes me feel sad. We should stop using ondata in http this is causing such weird stuff to happen...

[indutny]

@jerem can you try commit above? ^

[jerem]

It fixed my test case. 👏

[indutny]

Fixed in d534c2b

[indutny]

Oops, in ebc95f0