Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: use autoDestroy: true in incoming message #33035

Closed
wants to merge 7 commits into from

Conversation

dnlup
Copy link
Contributor

@dnlup dnlup commented Apr 24, 2020

Enable the default autoDestroy: true option in IncomingMessage.

Refactor _http_client and _http_server to remove any manual destroying/closing of IncomingMessage.
Refactor IncomingMessage destroy method to use the standard implementation of the stream module and move the abort logic
inside of it.

Ref:

#30625

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Apr 24, 2020
@dnlup dnlup force-pushed the _http_incoming_autodestroy branch from f4960e4 to 95d8354 Compare April 29, 2020 05:14
@ronag
Copy link
Member

ronag commented Apr 30, 2020

IncomingMessage overrides destroy() which kind of blocks any effort in this regards. That needs to IMO be fixed first.

@dnlup
Copy link
Contributor Author

dnlup commented May 1, 2020

IncomingMessage overrides destroy() which kind of blocks any effort in this regards. That needs to IMO be fixed first.

Thanks, @ronag, for the suggestion. I'll look into it. There is an issue with destroy for sure. When using keep-alive, we don't want to destroy the socket, and that's what's happening by just setting the option to true.

@dnlup dnlup force-pushed the _http_incoming_autodestroy branch from 95d8354 to 28da814 Compare May 4, 2020 08:28
@dnlup
Copy link
Contributor Author

dnlup commented May 6, 2020

Sorry if this is taking a long time, but I am trying to figure out what's wrong 🙏

Removing the override of destroy() resolves the EECONRESET errors, fewer tests are failing, but request is emitting close twice, and some connections seem to hang.

@dnlup dnlup force-pushed the _http_incoming_autodestroy branch 3 times, most recently from 336478c to 62e8aec Compare May 20, 2020 06:36
@dnlup dnlup marked this pull request as ready for review May 20, 2020 06:36
@dnlup dnlup force-pushed the _http_incoming_autodestroy branch from 62e8aec to 26a6e60 Compare May 20, 2020 07:09
@dnlup
Copy link
Contributor Author

dnlup commented May 20, 2020

The changes made here are overriding the ones you made recently, @ronag . I don't know if this approach is the best.

PS: just rebased against master on the last forced push.

@BridgeAR
Copy link
Member

@ronag PTAL

@BridgeAR BridgeAR requested a review from ronag May 27, 2020 23:56
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also try to remove all cases of manually managing destroyed and emitting 'close' on IncomingMessage. Having some kind of hybrid solution will make things hard to maintain IMO.

lib/_http_incoming.js Show resolved Hide resolved
@dnlup
Copy link
Contributor Author

dnlup commented May 28, 2020

Please also try to remove all cases of manually managing destroyed and emitting 'close' on IncomingMessage. Having some kind of hybrid solution will make things hard to maintain IMO.

You mean like in here? I agree, that is also the reason why the callback is not called in all cases in _destroy. I also agree that it is ugly, sorry. I wasn't sure I could change the _http_server.js parts.

Thanks for the pointers.

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
@dnlup
Copy link
Contributor Author

dnlup commented Jun 3, 2020

Do we need to keep this behavior? If we do, I think it would be better to implement the destroy logic directly in IncomingMessage and not reusing the general lib/internal/streams/destroy.js implementation as I did. Overriding that implementation causes a lot of tests to fail because they are expecting an unhandled error. Also, do you have anything against adding an abort method on IncomingMessage?

@ronag
Copy link
Member

ronag commented Jun 3, 2020

Do we need to keep this behavior?

Yes, just call destroy and move the logic there?

If we do, I think it would be better to implement the destroy logic directly in IncomingMessage and not reusing the general lib/internal/streams/destroy.js implementation as I did.

I disagree. We should implement it as a regular destroy.

Overriding that implementation causes a lot of tests to fail because they are expecting an unhandled error.

Can still be an unhandeld error?

Also, do you have anything against adding an abort method on IncomingMessage?

Yes, destroy should be abort. We just deprecated abort on ClientRequest in favor of destroy.

@dnlup
Copy link
Contributor Author

dnlup commented Jun 3, 2020

Yes, just call destroy and move the logic there?

Ok.

I disagree. We should implement it as a regular destroy.

I agree. The only difference I have seen with a regular destroy is that IncomingMessage emits an error only if there are listeners attached, at least when aborting.

Yes, destroy should be abort. We just deprecated abort on ClientRequest in favor of destroy.

Got it.

@ronag
Copy link
Member

ronag commented Jun 3, 2020

The only difference I have seen with a regular destroy is that IncomingMessage emits an error only if there are listeners attached, at least when aborting.

I can help with sorting this out. If you have any test that depends on this behavior just comment them out and we can take a look together when that's the only thing remaining.

@dnlup dnlup force-pushed the _http_incoming_autodestroy branch 4 times, most recently from 872f751 to 3231c42 Compare June 9, 2020 06:23
lib/_http_client.js Outdated Show resolved Hide resolved
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Add to the history table that the `destroyed` value returns `true` after
the incoming data is consumed.

Refs: #36617
Refs: #33035

PR-URL: #36641
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
Since 55e83cb
has changed the ordering of the close event, add a test case.
IncomingMessage will emit close before the response is sent in case the
server is consuming data from it.

Refs: #33035 (comment)

PR-URL: #36645
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@devinivy devinivy mentioned this pull request Feb 2, 2021
1 task
Jimbly added a commit to Jimbly/node-http-proxy that referenced this pull request Dec 8, 2021
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](https://nodejs.org/dist/latest-v16.x/docs/api/deprecations.html#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`.
aduh95 pushed a commit that referenced this pull request Apr 1, 2022
PR-URL: #42521
Fixes: #38924
Refs: #33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol pushed a commit to juanarbol/node that referenced this pull request Apr 5, 2022
PR-URL: nodejs#42521
Fixes: nodejs#38924
Refs: nodejs#33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol pushed a commit that referenced this pull request Apr 6, 2022
PR-URL: #42521
Fixes: #38924
Refs: #33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
xtx1130 pushed a commit to xtx1130/node that referenced this pull request Apr 25, 2022
PR-URL: nodejs#42521
Fixes: nodejs#38924
Refs: nodejs#33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
juanarbol pushed a commit that referenced this pull request May 31, 2022
PR-URL: #42521
Fixes: #38924
Refs: #33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams pushed a commit that referenced this pull request Jun 27, 2022
PR-URL: #42521
Fixes: #38924
Refs: #33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
targos pushed a commit that referenced this pull request Jul 11, 2022
PR-URL: #42521
Fixes: #38924
Refs: #33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
guangwong pushed a commit to noslate-project/node that referenced this pull request Oct 10, 2022
PR-URL: nodejs/node#42521
Fixes: nodejs/node#38924
Refs: nodejs/node#33035
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Beth Griggs <bgriggs@redhat.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Jimbly added a commit to Jimbly/node-http-proxy that referenced this pull request May 12, 2023
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](https://nodejs.org/dist/latest-v16.x/docs/api/deprecations.html#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`.
pachirel added a commit to pachirel/node-http-proxy that referenced this pull request Oct 16, 2023
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](https://nodejs.org/dist/latest-v16.x/docs/api/deprecations.html#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`.

The original commit is [here](http-party@058182a) by Jimbly.
muratso pushed a commit to ExodusMovement/node-http-proxy that referenced this pull request Apr 29, 2024
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](https://nodejs.org/dist/latest-v16.x/docs/api/deprecations.html#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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants