GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — dgram: support AbortSignal in createSocket by Linkgoron · Pull Request #37026 · nodejs/node

Conversation

@Linkgoron

@Linkgoron Linkgoron commented Jan 22, 2021

Copy link
Copy Markdown
Contributor

dgram: support AbortSignal in createSocket

Add AbortSignal support to dgram.createSocket. Added signal to the createSocket options, which calls close on the socket when aborted.

  • 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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot nodejs-github-bot added the dgram Issues and PRs related to UDP and the dgram module. label Jan 22, 2021
@Linkgoron
Linkgoron force-pushed the dgram-socket-abortcontroller branch from 5d1646c to 456f2c3 Compare January 22, 2021 21:18
benjamingr
benjamingr reviewed Jan 22, 2021
View reviewed changes
Comment thread lib/dgram.js Resolved

Uh oh!

There was an error while loading. Please reload this page.
benjamingr
benjamingr reviewed Jan 22, 2021
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Good job, left a question - overall lgtm with the question resolved

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Nitzan Uziely
dgram: support AbortSignal in createSocket
570575a
@Linkgoron
Linkgoron force-pushed the dgram-socket-abortcontroller branch from 456f2c3 to 570575a Compare January 22, 2021 21:38
benjamingr
benjamingr approved these changes Jan 22, 2021
View reviewed changes
Trott
Trott approved these changes Jan 23, 2021
View reviewed changes
jasnell
jasnell reviewed Jan 25, 2021
View reviewed changes
Comment thread lib/dgram.js
if (signal.aborted) {
onAborted();
} else {
signal.addEventListener('abort', onAborted);

@jasnell jasnell Jan 25, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Suggested change
signal.addEventListener('abort', onAborted);
signal.addEventListener('abort', onAborted, { once: true });

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Jan 25, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this is fine, once just creates the impression that it removes the abort listener on 'abort' but you'd need the removeEventListener anyway for the "server closed for another reason" case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nodejs-github-bot

nodejs-github-bot commented Jan 25, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35732/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
benjamingr pushed a commit that referenced this pull request Jan 25, 2021
Nitzan Uziely @benjamingr
dgram: support AbortSignal in createSocket
21c8c7e 
PR-URL: #37026
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@benjamingr

benjamingr commented Jan 25, 2021

Copy link
Copy Markdown
Member

Landed in 21c8c7e 🎉
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@benjamingr benjamingr closed this Jan 25, 2021
@benjamingr benjamingr added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 25, 2021
targos pushed a commit that referenced this pull request Feb 2, 2021
Nitzan Uziely @targos
dgram: support AbortSignal in createSocket
2c7ad38 
PR-URL: #37026
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Feb 2, 2021
v15.8.0 release proposal #37183
Merged
targos added a commit that referenced this pull request Feb 2, 2021
@targos
2021-02-02, Version 15.8.0 (Current)
5a59dde 
Notable changes:

crypto:
  * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997
  * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879
deps:
  * upgrade npm to 7.5.0 (Ruy Adorno) #37117
dgram:
  * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026
doc:
  * add Zijian Liu to collaborators (ZiJian Liu) #37075
esm:
  * deprecate legacy main lookup for modules (Guy Bedford) #36918
readline:
  * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662
  * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676

PR-URL: #37183
targos added a commit that referenced this pull request Feb 2, 2021
@targos
2021-02-02, Version 15.8.0 (Current)
bc1c428 
Notable changes:

crypto:
  * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997
  * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879
deps:
  * upgrade npm to 7.5.0 (Ruy Adorno) #37117
dgram:
  * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026
doc:
  * add Zijian Liu to collaborators (ZiJian Liu) #37075
esm:
  * deprecate legacy main lookup for modules (Guy Bedford) #36918
readline:
  * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662
  * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676

PR-URL: #37183
targos added a commit that referenced this pull request Feb 2, 2021
@targos
2021-02-02, Version 15.8.0 (Current)
211574b 
Notable changes:

crypto:
  * (SEMVER-MINOR) add generatePrime/checkPrime (James M Snell) #36997
  * (SEMVER-MINOR) experimental (Ed/X)25519/(Ed/X)448 support (James M Snell) #36879
deps:
  * upgrade npm to 7.5.0 (Ruy Adorno) #37117
dgram:
  * (SEMVER-MINOR) support AbortSignal in createSocket (Nitzan Uziely) #37026
doc:
  * add Zijian Liu to collaborators (ZiJian Liu) #37075
esm:
  * deprecate legacy main lookup for modules (Guy Bedford) #36918
readline:
  * (SEMVER-MINOR) add history event and option to set initial history (Mattias Runge-Broberg) #33662
  * (SEMVER-MINOR) add support for the AbortController to the question method (Mattias Runge-Broberg) #33676

PR-URL: #37183
@Linkgoron
Linkgoron deleted the dgram-socket-abortcontroller branch February 26, 2021 14:27
@targos targos added the backport-open-v14.x label Apr 24, 2021
@targos targos mentioned this pull request Apr 24, 2021
[v14.x] Backport AbortController and friends #38386
Closed
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
Nitzan Uziely @targos
dgram: support AbortSignal in createSocket
1b04e63 
PR-URL: nodejs#37026
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Apr 30, 2021
Nitzan Uziely @targos
dgram: support AbortSignal in createSocket
81cd06b 
PR-URL: #37026
Backport-PR-URL: #38386
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
@benjamingr benjamingr benjamingr approved these changes
@Trott Trott Trott approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

dgram Issues and PRs related to UDP and the dgram module. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@Linkgoron @nodejs-github-bot @benjamingr @jasnell @Trott @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.