fix: socket back pressure memory leak

[ronag]

Fixes: #434

[ronag]

@szmarczak

[ronag]

@mcollina I would very much like to make a release out of this as soon as possible.

[mcollina]

lgtm

[mcollina]

go for it!

[szmarczak]

Is socket[kBuffer] supposed to be null?

[ronag]

socket[kBuffer]? I don't think we have that?

[szmarczak]

Node.js lib does:

https://github.com/nodejs/node/blob/62c44af5c5f2964849bba92ff90c32e867d63f83/lib/net.js#L599-L605

[ronag]

Hm, I don't understand how that works at the moment unfortunately.

[ronag]

@mcollina Do we know someone that knows these internals well and we could ask for a quick review in regards to consume and socket.

[ronag]

Seems like whatever kBuffer does it's opt-in. nodejs/node#25436

[szmarczak]

I think @mcollina was right here: https://github.com/nodejs/node/pull/25436/files#diff-e6ef024c3775d787c38487a6309e491dR602 Overriding [public] Stream API sounds like a mistake.

[szmarczak]

Why does it override .read() at all? Shouldn't it set ._read() instead?

[ronag]

Why does it override .read() at all? Shouldn't it set ._read() instead?

What overrides it?

[ronag]

I think @mcollina was right here: nodejs/node#25436 (files) Overriding [public] Stream API sounds like a mistake.

Where is that comment?

[szmarczak]

Where is that comment?

lib/net.js after line 603

If you click the (files) link it should scroll down to the comment. It is marked as resolved so you need to click Show conversation.

Screenshot

What overrides it?

I'm still talking about Node.js core (lib/net.js file) 😅

Shouldn't it be:

-Socket.prototype.read = function(n) {
+Socket.prototype._read = function(n) {
  if (this[kBuffer] && !this.connecting && this._handle &&
      !this._handle.reading) {
    tryReadStart(this);
  }
- return stream.Duplex.prototype.read.call(this, n);
};

[ronag]

@szmarczak Yea, that's a bit weird... does it affect us though?