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: fix double AbortSignal registration #37730

Merged

Conversation

Linkgoron
Copy link
Member

@Linkgoron Linkgoron commented Mar 12, 2021

It looks like #36431 added support for AbortSignal to net.createConnection and net.connect (will add tests/docs in a separate PR). This caused "double registration" to the AbortSignal. This PR fixes the issue by removing the signal from the internal calls to net.

Also added tests for http.agent

@benjamingr

@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. needs-ci PRs that need a full CI run. labels Mar 12, 2021
@Linkgoron Linkgoron force-pushed the http-fix-double-abort-signal-registration branch from fca3195 to 5e0e0d2 Compare March 12, 2021 21:09
@Linkgoron Linkgoron force-pushed the http-fix-double-abort-signal-registration branch 3 times, most recently from 43d4392 to 5fae5af Compare March 13, 2021 01:36
@Linkgoron Linkgoron force-pushed the http-fix-double-abort-signal-registration branch 2 times, most recently from fc42d56 to 38771f4 Compare March 13, 2021 23:56
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@Linkgoron Linkgoron force-pushed the http-fix-double-abort-signal-registration branch from 38771f4 to ee27a86 Compare March 19, 2021 21:09
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

Fix an issue where AbortSignals are registered twice

PR-URL: nodejs#37730
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott Trott force-pushed the http-fix-double-abort-signal-registration branch from ee27a86 to 711e210 Compare March 20, 2021 19:53
@Trott Trott merged commit 711e210 into nodejs:master Mar 20, 2021
@Trott
Copy link
Member

Trott commented Mar 20, 2021

Landed in 711e210

ruyadorno pushed a commit that referenced this pull request Mar 24, 2021
Fix an issue where AbortSignals are registered twice

PR-URL: #37730
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Mar 30, 2021
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. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants