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.

Sign up for GitHub

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

Jump to bottom

http: fix ClientRequest unhandled errors #36970

Closed
wants to merge 7 commits into from
Closed

http: fix ClientRequest unhandled errors #36970

wants to merge 7 commits into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 16, 2021 •
edited
Loading

ClientRequest could somtimes cause an unhandled error from socket.

Fixes: #36931

@ronag
http: fix ClientRequest unhandled errors
4fe8533 
ClientRequest could someone cause an unhandled error
from socket.

Fixes: nodejs#36931
@ronag ronag added http Issues or PRs related to the http subsystem. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 16, 2021
@ronag ronag requested a review from lpinca January 16, 2021 17:19
ronag
ronag commented Jan 16, 2021
View reviewed changes
lib/_http_client.js Outdated Show resolved Hide resolved
@ronag ronag requested a review from dnlup January 16, 2021 17:21
lpinca
lpinca reviewed Jan 16, 2021
View reviewed changes
lib/_http_agent.js Outdated Show resolved Hide resolved
lpinca
lpinca approved these changes Jan 16, 2021
View reviewed changes
test/parallel/test-http-client-abort3.js Outdated Show resolved Hide resolved
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 16, 2021
@ronag
Copy link
Member Author

ronag commented Jan 16, 2021

@nodejs/http

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35583/

mcollina
mcollina approved these changes Jan 17, 2021
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 17, 2021
@ronag
Copy link
Member Author

ronag commented Jan 17, 2021

Test needs improvment.

@dnlup
Copy link
Contributor

dnlup commented Jan 18, 2021

I cannot reproduce the timeout error on test/parallel/test-cluster-primary-kill.js, it might be a temporary error.

dnlup
dnlup reviewed Jan 18, 2021
View reviewed changes
Copy link
Contributor

@dnlup dnlup left a comment

Choose a reason for hiding this comment

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

lgtm

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 3, 2021
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35918/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35919/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35920/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35921/

ronag added a commit that referenced this pull request Feb 3, 2021
@ronag
http: fix ClientRequest unhandled errors
fe43bd8 
ClientRequest could someone cause an unhandled error
from socket.

Fixes: #36931

PR-URL: #36970
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@ronag
Copy link
Member Author

ronag commented Feb 3, 2021

Landed in fe43bd8

@ronag ronag closed this Feb 3, 2021
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@ronag @danielleadams
http: fix ClientRequest unhandled errors
4cdc5ea 
ClientRequest could someone cause an unhandled error
from socket.

Fixes: #36931

PR-URL: #36970
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
This was referenced Feb 16, 2021
Closed
Merged
@panva panva removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 21, 2023
@Changqing-JING Changqing-JING mentioned this pull request Jun 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@dnlup dnlup dnlup approved these changes

@benjamingr benjamingr benjamingr approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unhandled 'error' event on aborted request
8 participants
@ronag @nodejs-github-bot @dnlup @mcollina @benjamingr @lpinca @panva @targos