Fix 'aborted' detection on Node v15.5.0+

[Jimbly]

This fixes a major memory leak I encountered usign this on Node v16.
Doing a binary search of node versions, the problem appears to originate in v15.5.0 as a result of nodejs/node#33035, however later changes have quietly completely removed the 'aborted' event that http-proxy relies on, and later added a deprecation note about it (which seems to actually be incorrect).
Despite what the notes about DEP0156 say, the only way I could get this module working reliably again was to replace req.on('aborted') with instead checking res.on('close') and looking at res.writeableFinished.

For a very easy test, load any video file over the proxy, and seek backwards in Chrome. For example, run the following:

const httpProxy = require('http-proxy');

httpProxy.createProxyServer({
  target: 'http://files.dashingstrike.com.s3.amazonaws.com',
  changeOrigin: true,
}).listen(8080);

And then open http://localhost:8080/SplodyGameplayLong.mp4, and then seek backwards in the file. Chrome will abort hundreds of requests (each of which is requesting ~32MB), which, if not properly aborted, will leak connections as well as hundreds of MBs of TCP kernel memory per second (quickly bricking your instance, making it unreachable even by SSH, as I found out -_-). On Linux, you can view TCP kernel memory used with cat /proc/net/sockstat (mem is count of 4K pages), or ss -atn state big if you have ss installed.

[RemiBou]

Hi @Jimbly any plan to merge this ?

[Jimbly]

Hi @Jimbly any plan to merge this ?

I'm not the one who can merge to this repository, that would be a question for the maintainers of node-http-proxy.

Since there was no response here (abandoned project?) I had to publish my fork for some other projects though, and you can use that in the meantime: http-proxy-node16

[RemiBou]

Thanks I'll checkit out ! Great work :) Le mar. 5 juil. 2022 à 14:59, Jimb Esser ***@***.***> a écrit :

…

Hi @Jimbly <https://github.com/Jimbly> any plan to merge this ? I'm not the one who can merge to this repository, that would be a question for the maintainers of node-http-proxy. Since there was no response here (abandoned project?) I had to publish my fork for some other projects though, and you can use that in the meantime: http-proxy-node16 <https://www.npmjs.com/package/http-proxy-node16> — Reply to this email directly, view it on GitHub <#1559 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKLY6QUK4NAC42H5KKLTKDVSQWSRANCNFSM5JUOB7TQ> . You are receiving this because you commented.Message ID: ***@***.***>

[craftzdog]

This patch worked! Thanks for the fix

[likev]

The response 'close' event and the 'destroy' method are the correct alternatives
#1580

[anthonyalayo]

Is the maintainer gone?

[Jimbly]

No maintainers as far as I can tell. I've (just now) pushed an update to http-proxy-node16 so that it correctly links back to my fork instead of this repo, if anyone wants to redirect PRs to that repository, I can potentially merge them and update my fork (with the caveat that I don't really know any of the internals here ^_^).

[cassepipe]

Does this affect all versions above 16 or just 16 ?

Is this why I have a

{"statusCode":403,"message":"sse already opened","error":"Forbidden"}

... when using vite ?

[Jimbly]

This affects all versions 16 and above, it's due to a low-level, non-backwards-compatible change in Node. No idea if it would have anything to do with that SSE error, though you can try swapping out for http-proxy-node16 and see if it fixes it, it should be otherwise the same package.

[cassepipe]

This seems like a no-brainer to merge this. Is there any issues with it @indexzero @jsmylnycky ?

You could either merge this one which is apparently backwards compatible with older versions of node or this drop backwards compatibility with this shorter one #1580.

Then you can kill two merge request with one stone and close whichever you don't merge.

[callumgare]

Super minor suggestion but when I first glanced at this based on the name I presumed it was a function for checking if a connection had been aborted. So maybe something like "supportsAbortedEvent" might be a bit of a clearer name?

[Jimbly]

Sounds good, changed!

[pickeloe5]

My website was crashing while I was advertising it which made me very upset.

This P.R. fixes the crash, thank you very much Jimbly.