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: abortIncoming only on socket close #36821

Closed
wants to merge 1 commit into from
Closed

http: abortIncoming only on socket close #36821

wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Jan 6, 2021

Don't call abortIncoming twice for same socket, i.e. both during 'end' and 'close'. Also does some minor cleanup.

@ronag ronag added the http Issues or PRs related to the http subsystem. label Jan 6, 2021
@ronag ronag requested a review from BridgeAR January 6, 2021 20:50
@ronag ronag force-pushed the http-abort branch 2 times, most recently from 5f9df3b to 55c02b6 Compare January 6, 2021 20:51
@ronag
http: abortIncoming only on socket close
43fc062 
Don't call abortIncombin twice for same socket, i.e. both during
'end' and 'close'.
@ronag
Copy link
Member Author

ronag commented Jan 9, 2021

@nodejs/http

dnlup
dnlup approved these changes Jan 10, 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.

close will always be emitted even if end is emitted, I agree.

@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 10, 2021
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 10, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@dnlup
Copy link
Contributor

dnlup commented Jan 11, 2021

node-api/test_worker_terminate_finalization/test should be unrelated to these changes, I am not sure why it keeps failing.

@nodejs-github-bot
Copy link
Collaborator

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

dnlup
dnlup approved these changes Jan 13, 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

@dnlup dnlup added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jan 13, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@dnlup
Copy link
Contributor

dnlup commented Jan 13, 2021

node-api/test_worker_terminate_finalization/test should be unrelated to these changes, I am not sure why it keeps failing.

After reading #36868, I think it should be safe to land this even if that test fails.

If there are no objections, I will proceed with landing this tomorrow. If I understand correctly from here, one collaborator approval should be enough if the PR has been open for more than days and has no objections.

@ronag
Copy link
Member Author

ronag commented Jan 13, 2021

We need two approval for landing though?

@dnlup
Copy link
Contributor

dnlup commented Jan 14, 2021

We need two approval for landing though?

The linked section of the docs says that if a PR is open for more than 7 days, then one approval is enough. But I can't say I have seen PR merged with one approval though.

On second thought, let's wait for a second approval. Keeping this open might also help to debug the failing test, I think.

@ronag
Copy link
Member Author

ronag commented Jan 14, 2021

The linked section of the docs says that if a PR is open for more than 7 days, then one approval is enough.

Ah, that's news for me. Thanks for the heads up.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@jasnell
Copy link
Member

jasnell commented Jan 18, 2021

Landed in 708728d

@jasnell jasnell closed this Jan 18, 2021
jasnell pushed a commit that referenced this pull request Jan 18, 2021
@ronag @jasnell
http: abortIncoming only on socket close
708728d 
Don't call abortIncombin twice for same socket, i.e. both during
'end' and 'close'.

PR-URL: #36821
Reviewed-By: James M Snell <[email protected]>
ruyadorno pushed a commit that referenced this pull request Jan 22, 2021
@ronag @ruyadorno
http: abortIncoming only on socket close
82c05f5 
Don't call abortIncombin twice for same socket, i.e. both during
'end' and 'close'.

PR-URL: #36821
Reviewed-By: James M Snell <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Jan 22, 2021
Merged
ruyadorno pushed a commit that referenced this pull request Jan 25, 2021
@ronag @ruyadorno
http: abortIncoming only on socket close
f07e1c9 
Don't call abortIncombin twice for same socket, i.e. both during
'end' and 'close'.

PR-URL: #36821
Reviewed-By: James M Snell <[email protected]>
@devinivy devinivy mentioned this pull request Feb 2, 2021
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnlup dnlup dnlup approved these changes

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

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.
5 participants
@ronag @nodejs-github-bot @dnlup @jasnell @targos