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

dgram: sync the old handle state to new handle #46041

Merged
merged 1 commit into from
Jan 3, 2023
Merged

dgram: sync the old handle state to new handle #46041

merged 1 commit into from
Jan 3, 2023

Conversation

theanarkh
Copy link
Contributor

Sync the old handle state to new handle Or the unref UDP socket will keep the loop alive.

  • 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 dgram Issues and PRs related to the dgram subsystem / UDP. needs-ci PRs that need a full CI run. labels Jan 1, 2023
mcollina
mcollina approved these changes Jan 1, 2023
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 1, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented Jan 1, 2023

The added test fails on Windows: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/18447/RUN_SUBSET=3,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/test/parallel_test_dgram_unref_in_cluster/

ShogunPanda
ShogunPanda approved these changes Jan 1, 2023
View reviewed changes
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
Copy link
Contributor Author

The added test fails on Windows: https://ci.nodejs.org/job/node-test-binary-windows-js-suites/18447/RUN_SUBSET=3,nodes=win2012r2-COMPILED_BY-vs2019-x86/testReport/junit/(root)/test/parallel_test_dgram_unref_in_cluster/

Yes, i have updated the test, and test it again.

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

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

@theanarkh
Copy link
Contributor Author

using dgram in cluster is currently not supported on Windows.

@github-actions github-actions bot mentioned this pull request Jan 2, 2023
Open
22 tasks
@nodejs-github-bot
Copy link
Collaborator

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

RaisinTen
RaisinTen approved these changes Jan 2, 2023
View reviewed changes
test/parallel/test-dgram-unref-in-cluster.js Outdated Show resolved Hide resolved
@theanarkh theanarkh added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 2, 2023
@nodejs-github-bot
Copy link
Collaborator

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

juanarbol
juanarbol approved these changes Jan 2, 2023
View reviewed changes
Copy link
Member

@juanarbol juanarbol 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 author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 2, 2023
@github-actions github-actions bot mentioned this pull request Jan 3, 2023
Open
23 tasks
@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 3, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 3, 2023
@nodejs-github-bot nodejs-github-bot merged commit 6d49f46 into nodejs:main Jan 3, 2023
@nodejs-github-bot
Copy link
Collaborator

Landed in 6d49f46

This was referenced Jan 4, 2023
Open
Open
RafaelGSS pushed a commit to RafaelGSS/node that referenced this pull request Jan 17, 2023
@theanarkh @RafaelGSS
dgram: sync the old handle state to new handle
7adff4d 
PR-URL: nodejs#46041
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Jan 20, 2023
@theanarkh @RafaelGSS
dgram: sync the old handle state to new handle
38dd506 
PR-URL: #46041
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 20, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@theanarkh @juanarbol
dgram: sync the old handle state to new handle
eb52663 
PR-URL: #46041
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
@juanarbol juanarbol mentioned this pull request Jan 28, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 31, 2023
@theanarkh @juanarbol
dgram: sync the old handle state to new handle
8de6425 
PR-URL: #46041
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@juanarbol juanarbol juanarbol approved these changes

@cjihrig cjihrig cjihrig approved these changes

@addaleax addaleax addaleax approved these changes

@ShogunPanda ShogunPanda ShogunPanda 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. dgram Issues and PRs related to the dgram subsystem / UDP. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@theanarkh @nodejs-github-bot @targos @mcollina @ShogunPanda @addaleax @cjihrig @juanarbol @RaisinTen @aduh95