Skip to content

dgram: make UDPWrap more reusable - #31871

Closed
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:reusable-udpwrap
Closed

dgram: make UDPWrap more reusable#31871
jasnell wants to merge 2 commits into
nodejs:masterfrom
jasnell:reusable-udpwrap

Conversation

@jasnell

@jasnelljasnell commented Feb 19, 2020

Copy link
Copy Markdown
Member

Authored by @addaleax and originally landed in the QUIC repo. Separating out from the QUIC PR. While this does not have any QUIC specific bits, QUIC would be the only current use of it so it would likely be best to land this PR in conjunction with the QUIC PR. Separating out only to make it easier to review.

Allow using the handle more directly for I/O in other parts of the codebase.

Original review metadata:

 PR-URL: https://github.com/nodejs/quic/pull/165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Feb 19, 2020
mscdex
mscdex reviewed Feb 19, 2020
View reviewed changes
Comment threadlib/dgram.js Outdated
bnoordhuis
bnoordhuis reviewed Feb 20, 2020
View reviewed changes
Comment threadsrc/udp_wrap.cc Outdated
Comment threadsrc/udp_wrap.cc Outdated
Comment threadsrc/udp_wrap.cc Outdated
@jasnell

jasnell commented Feb 24, 2020

Copy link
Copy Markdown
MemberAuthor

@addaleax ... given that you wrote the original change here, do you have thoughts on the suggestions from @mscdex and @bnoordhuis ?

@addaleax

addaleax commented Feb 24, 2020

Copy link
Copy Markdown
Member

@jasnell Feel free to address Ben’s comments (or if you want, I can do that later this evening)

@jasnell

jasnell commented Feb 29, 2020

Copy link
Copy Markdown
MemberAuthor

Waiting for #31960 to land then will rebase this and update it to incorporate those changes.

@jasnell

jasnell commented Mar 2, 2020

Copy link
Copy Markdown
MemberAuthor

Rebased now that #31960 landed. @bnoordhuis and @addaleax, please take another look

addaleax
addaleax reviewed Mar 2, 2020
View reviewed changes
Comment threadsrc/udp_wrap.h Outdated
@jasnelljasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 2, 2020
@nodejs-github-bot

nodejs-github-bot commented Mar 2, 2020

Copy link
Copy Markdown
Collaborator

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

@addaleax@jasnell
dgram: make UDPWrap more reusable
347ea10
Allow using the handle more directly for I/O in other parts of
the codebase.
Originally landed in the QUIC repo
Original review metadata:
```
PR-URL: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
```
Signed-off-by: James M Snell <jasnell@gmail.com>
@nodejs-github-bot

nodejs-github-bot commented Mar 2, 2020

Copy link
Copy Markdown
Collaborator

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

jasnell pushed a commit that referenced this pull request Mar 3, 2020
@addaleax@jasnell
dgram: make UDPWrap more reusable
43b7142
Allow using the handle more directly for I/O in other parts of
the codebase.
Originally landed in the QUIC repo
Original review metadata:
```
PR-URL: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
```
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #31871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@jasnell

jasnell commented Mar 3, 2020

Copy link
Copy Markdown
MemberAuthor

Landed in 43b7142

@jasnelljasnell closed this Mar 3, 2020
MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
@addaleax@MylesBorins
dgram: make UDPWrap more reusable
4dc59b9
Allow using the handle more directly for I/O in other parts of
the codebase.
Originally landed in the QUIC repo
Original review metadata:
```
PR-URL: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
```
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #31871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@MylesBorinsMylesBorins mentioned this pull request Mar 4, 2020
Merged
@targos

targos commented Apr 20, 2020

Copy link
Copy Markdown
Member

Depends on #31960 to land on v12.x

@targostargos mentioned this pull request Apr 20, 2020
Closed
3 tasks
@targostargos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
@addaleax@targos
dgram: make UDPWrap more reusable
ea17cf6
Allow using the handle more directly for I/O in other parts of
the codebase.
Originally landed in the QUIC repo
Original review metadata:
```
PR-URL: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
```
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: nodejs#31871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
targos pushed a commit that referenced this pull request Apr 28, 2020
@addaleax@targos
dgram: make UDPWrap more reusable
322a998
Allow using the handle more directly for I/O in other parts of
the codebase.
Originally landed in the QUIC repo
Original review metadata:
```
PR-URL: nodejs/quic#165
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
```
Signed-off-by: James M Snell <jasnell@gmail.com>
PR-URL: #31871
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@targostargos mentioned this pull request May 2, 2020
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnoordhuisbnoordhuisbnoordhuis approved these changes
@addaleaxaddaleaxaddaleax approved these changes
+1 more reviewer
@mscdexmscdexmscdex left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++Issues and PRs that require attention from people who are familiar with C++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jasnell@addaleax@nodejs-github-bot@targos@mscdex@bnoordhuis