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

stream: autoDestroy: true #30625

Closed
4 of 6 tasks
ronag opened this issue Nov 24, 2019 · 7 comments
Closed
4 of 6 tasks

stream: autoDestroy: true #30625

ronag opened this issue Nov 24, 2019 · 7 comments
Labels
meta Issues and PRs related to the general management of the project. stream Issues and PRs related to the stream subsystem.

Comments

@ronag
Copy link
Member

ronag commented Nov 24, 2019

This is a tracking issue for migrating all node core modules to use autoDestroy: true

@ronag
Copy link
Member Author

ronag commented Nov 24, 2019

I think to a certain degree this would be simpler if done at the same time as migrating to construct (if that gets merged).

@ronag
Copy link
Member Author

ronag commented Nov 24, 2019

Related #30621, #30623

@ronag ronag added meta Issues and PRs related to the general management of the project. stream Issues and PRs related to the stream subsystem. labels Feb 9, 2020
@ronag
Copy link
Member Author

ronag commented Apr 19, 2020

Crossed out http2. Does not seem feasible to move to http2. Unsure if http2 lifecycle adheres to the default streams life cycle.

@dnlup
Copy link
Contributor

dnlup commented Apr 22, 2020

Is _http_ougoing.js needed? I think _http_incoming.js can be marked as done (#30623)

@ronag
Copy link
Member Author

ronag commented Apr 22, 2020

Is _http_ougoing.js needed?

Is not a real stream, yet.

I think _http_incoming.js can be marked as done (#30623)

No, it's still not using autoDestroy, https://github.com/nodejs/node/pull/30623/files#diff-f7d8a311cc7ac30522e414465bda2229R51

@dnlup
Copy link
Contributor

dnlup commented Apr 22, 2020

Thank you @ronag for the clarification. I misinterpreted this issue by considering that the modules should have been just refactored to be aware of the new default autoDestroy: true option in streams. I would like to help with _http_incoming.js then, I'll try to work on it.

nodejs-github-bot pushed a commit that referenced this issue Dec 17, 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 early termination
event emitting inside of it.

PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Dec 17, 2020
Destroy the underlying socket only if it is not ready destroyed. Wait
for the stream to finish in that case.

PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Dec 17, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Dec 17, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Dec 17, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Dec 17, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
nodejs-github-bot pushed a commit that referenced this issue Dec 17, 2020
Test uncaught exceptions when destroying IncomingMessage.

PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@ronag
Copy link
Member Author

ronag commented Dec 17, 2020

This has been fixed

@ronag ronag closed this as completed Dec 17, 2020
targos pushed a commit that referenced this issue Dec 21, 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 early termination
event emitting inside of it.

PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Dec 21, 2020
Destroy the underlying socket only if it is not ready destroyed. Wait
for the stream to finish in that case.

PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Dec 21, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Dec 21, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Dec 21, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Dec 21, 2020
PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this issue Dec 21, 2020
Test uncaught exceptions when destroying IncomingMessage.

PR-URL: #33035
Refs: #30625
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta Issues and PRs related to the general management of the project. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

2 participants