Skip to content

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

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
theanarkh:fix_udp_state
Jan 3, 2023
Merged

dgram: sync the old handle state to new handle#46041
nodejs-github-bot merged 1 commit intonodejs:mainfrom
theanarkh:fix_udp_state

Conversation

@theanarkh
Copy link
Contributor

@theanarkh theanarkh commented Jan 1, 2023

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

nodejs-github-bot commented Jan 1, 2023

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

nodejs-github-bot commented Jan 1, 2023

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

theanarkh 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/

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

nodejs-github-bot commented Jan 1, 2023

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

@theanarkh
Copy link
Contributor Author

theanarkh commented Jan 1, 2023

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

nodejs-github-bot commented Jan 2, 2023

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

RaisinTen
RaisinTen approved these changes Jan 2, 2023
View reviewed changes
@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

nodejs-github-bot commented Jan 2, 2023

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

nodejs-github-bot commented Jan 3, 2023

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

@mcollina mcollina mcollina approved these changes

@ShogunPanda ShogunPanda ShogunPanda approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@juanarbol juanarbol juanarbol approved these changes

@RaisinTen RaisinTen RaisinTen 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