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

net: fix connect crash when call destroy in lookup handler #51826

Conversation

theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Feb 21, 2024

fix: #50841

cc @ShogunPanda

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Feb 21, 2024
@theanarkh theanarkh force-pushed the fix_connect_crash_when_call_destroy_in_lookup_handler branch from 6a7c817 to 18718f4 Compare February 21, 2024 18:11
Copy link
Contributor

@ShogunPanda ShogunPanda left a comment

Choose a reason for hiding this comment

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

LGTM!

@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2024
@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2024
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

test/parallel/test-destroy-socket-in-lookup.js Outdated Show resolved Hide resolved
test/parallel/test-destroy-socket-in-lookup.js Outdated Show resolved Hide resolved
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

@theanarkh theanarkh force-pushed the fix_connect_crash_when_call_destroy_in_lookup_handler branch from 18718f4 to 23b04ca Compare February 23, 2024 10:14
@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 23, 2024
@nodejs-github-bot
Copy link
Collaborator

@theanarkh theanarkh added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Feb 23, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 24, 2024
@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 24, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 24, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51826
✔  Done loading data for nodejs/node/pull/51826
----------------------------------- PR info ------------------------------------
Title      net: fix connect crash when call destroy in lookup handler (#51826)
Author     theanarkh  (@theanarkh)
Branch     theanarkh:fix_connect_crash_when_call_destroy_in_lookup_handler -> nodejs:main
Labels     net, author ready, needs-ci
Commits    1
 - net: fix connect crash when call destroy in lookup handler
Committers 1
 - theanarkh 
PR-URL: https://github.com/nodejs/node/pull/51826
Fixes: https://github.com/nodejs/node/issues/50841
Reviewed-By: Paolo Insogna 
Reviewed-By: Luigi Pinca 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51826
Fixes: https://github.com/nodejs/node/issues/50841
Reviewed-By: Paolo Insogna 
Reviewed-By: Luigi Pinca 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - net: fix connect crash when call destroy in lookup handler
   ℹ  This PR was created on Wed, 21 Feb 2024 17:12:43 GMT
   ✔  Approvals: 3
   ✔  - Paolo Insogna (@ShogunPanda) (TSC): https://github.com/nodejs/node/pull/51826#pullrequestreview-1895223170
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51826#pullrequestreview-1896819310
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/51826#pullrequestreview-1897763462
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-02-23T10:16:28Z: https://ci.nodejs.org/job/node-test-pull-request/57340/
- Querying data for job/node-test-pull-request/57340/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/8028025584

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 24, 2024
@theanarkh theanarkh added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 27, 2024
@nodejs-github-bot nodejs-github-bot merged commit 292016c into nodejs:main Feb 27, 2024
68 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 292016c

marco-ippolito pushed a commit that referenced this pull request Feb 29, 2024
PR-URL: #51826
Fixes: #50841
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
marco-ippolito pushed a commit that referenced this pull request Feb 29, 2024
PR-URL: #51826
Fixes: #50841
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51826
Fixes: #50841
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51826
Fixes: #50841
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#51826
Fixes: nodejs#50841
Reviewed-By: Paolo Insogna <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Jiawen Geng <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-failed An error occurred while landing this pull request using GitHub Actions. needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Node js internal assertion error in socket
6 participants