FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

dns: add a cancel() method to the promise Resolver by szmarczak · Pull Request #33099 · nodejs/node · GitHub

/ node Public

dns: add a cancel() method to the promise Resolver - #33099

Closed
szmarczak wants to merge 3 commits into
nodejs:masterfrom
szmarczak:resolver-cancel
ConversationCommits (3)ChecksFiles changed
Closed

dns: add a cancel() method to the promise Resolver#33099
szmarczak wants to merge 3 commits into
nodejs:masterfrom
szmarczak:resolver-cancel

Conversation

szmarczak commented Apr 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

Fixes #33091

Checklist
  • 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 added the dns Issues and PRs related to the dns subsystem. label Apr 27, 2020
addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Apr 27, 2020

addaleax commented Apr 27, 2020

Copy link
Copy Markdown
Member

The commit message should probably indicate in some way that this only affects dns.promises, I think?

targos reviewed Apr 27, 2020
View reviewed changes

targos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please add something to the documentation

szmarczak changed the title dns: add resolver.cancel() dns: add a cancel() method to the promise DNS resolver Apr 27, 2020

szmarczak commented Apr 27, 2020

Copy link
Copy Markdown
Member Author

The commit message should probably indicate in some way that this only affects dns.promises, I think?

Fixed.

Please add something to the documentation

Will do soon. That's why I've left the check unticked. I'm aware of this already :)

rickyes reviewed Apr 27, 2020
View reviewed changes
Comment thread test/parallel/test-dns-promise-channel-cancel.js Outdated

rickyes Apr 27, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is it more accurate to rename to test-dns-channel-cancel-promises.

jasnell reviewed Apr 27, 2020
View reviewed changes
Comment thread test/parallel/test-dns-promise-channel-cancel.js Outdated
szmarczak changed the title dns: add a cancel() method to the promise DNS resolver dns: add a cancel() method to the promise Resolver Apr 27, 2020
rickyes reviewed Apr 27, 2020
View reviewed changes
Comment thread doc/api/dns.md Outdated
szmarczak requested a review from jasnell April 27, 2020 19:22
addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 29, 2020

nodejs-github-bot commented Apr 29, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented Apr 30, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 1, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 7, 2020

Copy link
Copy Markdown
Collaborator

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

szmarczak commented May 8, 2020

Copy link
Copy Markdown
Member Author

This is good to go... I guess?

addaleax commented May 9, 2020

Copy link
Copy Markdown
Member

@szmarczak Yes, once CI is green 👍

nodejs-github-bot commented May 9, 2020

Copy link
Copy Markdown
Collaborator

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

nodejs-github-bot commented May 9, 2020

Copy link
Copy Markdown
Collaborator

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

targos removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label May 10, 2020

targos commented May 10, 2020

Copy link
Copy Markdown
Member

Two tests fail on Windows:

parallel/test-dns-promise-channel-cancel:

Mismatched <anonymous> function calls. Expected exactly 11, actual 2.
    at Proxy.mustCall (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:330:10)
    at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-dns-promise-channel-cancel.js:50:29)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)
    at Module.load (internal/modules/cjs/loader.js:1040:32)
    at Function.Module._load (internal/modules/cjs/loader.js:929:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

parallel/test-dns-channel-cancel:

Mismatched <anonymous> function calls. Expected exactly 11, actual 1.
    at Proxy.mustCall (C:\workspace\node-test-binary-windows-js-suites\node\test\common\index.js:330:10)
    at Object.<anonymous> (C:\workspace\node-test-binary-windows-js-suites\node\test\parallel\test-dns-channel-cancel.js:41:29)
    at Module._compile (internal/modules/cjs/loader.js:1176:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1196:10)
    at Module.load (internal/modules/cjs/loader.js:1040:32)
    at Function.Module._load (internal/modules/cjs/loader.js:929:14)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:71:12)
    at internal/main/run_main_module.js:17:47

BridgeAR commented May 27, 2020

Copy link
Copy Markdown
Member

Ping @szmarczak

szmarczak commented May 28, 2020

Copy link
Copy Markdown
Member Author

I don't have the time to code now, will do in three weeks. Preparing for maturity exams rn.

aduh95 commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

@szmarczak can you drop the merge commit and rebase instead please? The merge commit break our tooling, the CI can't start.

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. and removed stalled Issues and PRs that are stalled. labels Nov 12, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 12, 2020

nodejs-github-bot commented Nov 12, 2020

Copy link
Copy Markdown
Collaborator

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

szmarczak commented Nov 13, 2020

Copy link
Copy Markdown
Member Author

Hmm... For some reason it still fails on Windows. The server receives only two queries, but on Linux it's 11. Will investigate more in a few hours.

szmarczak commented Nov 13, 2020

Copy link
Copy Markdown
Member Author

@aduh95 Can you please request another CI build? All GitHub Action jobs passed, so I believe the CI will pass too.

aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 13, 2020

This comment has been minimized.

Sign in to view

szmarczak commented Nov 14, 2020

Copy link
Copy Markdown
Member Author

@aduh95 Good news! All tests passed! Jenkins is reporting that test.parallel/test-http2-client-jsstream-destroy is failing with Error [ERR_STREAM_DESTROYED]: Cannot call write after a stream was destroyed on macOS/10.14, which is totally unrelated with this PR. I think this PR is good to merge now.

nodejs-github-bot commented Nov 14, 2020

Copy link
Copy Markdown
Collaborator

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

aduh95 commented Nov 14, 2020

Copy link
Copy Markdown
Contributor

Landed in 9ea0fae

aduh95 closed this Nov 14, 2020
aduh95 pushed a commit that referenced this pull request Nov 14, 2020
dns: add a cancel() method to the promise Resolver
9ea0fae 
PR-URL: #33099
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
codebytere pushed a commit that referenced this pull request Nov 22, 2020
dns: add a cancel() method to the promise Resolver
6349b1d 
PR-URL: #33099
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
codebytere added a commit that referenced this pull request Nov 22, 2020
2020-11-24, Version 15.3.0 (Current)
ac213a0 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: TODO
codebytere mentioned this pull request Nov 22, 2020
Merged
codebytere added a commit that referenced this pull request Nov 24, 2020
2020-11-24, Version 15.3.0 (Current)
636a649 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: TODO
codebytere added a commit that referenced this pull request Nov 24, 2020
2020-11-24, Version 15.3.0 (Current)
63e7dc1 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: #36232
codebytere added a commit that referenced this pull request Nov 24, 2020
2020-11-24, Version 15.3.0 (Current)
7b5b8ef 
Notable changes:

dns:
  * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099
events:
  * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001
http:
  * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048
http2:
  * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978
lib:
  * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716
path:
  * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962
readline:
  * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675
src:
  * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940
util:
  * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055

PR-URL: #36232
targos pushed a commit that referenced this pull request May 1, 2021
dns: add a cancel() method to the promise Resolver
8715462 
PR-URL: #33099
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
danielleadams mentioned this pull request May 3, 2021
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
addaleax addaleax approved these changes
cjihrig cjihrig approved these changes
aduh95 aduh95 approved these changes
juanarbol juanarbol approved these changes
+2 more reviewers
targos targos left review comments
rickyes rickyes left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. dns Issues and PRs related to the dns subsystem. 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.

dns.promises.Resolver does not have a .cancel() method

10 participants

Back | FazBrowse Home | New Git URL