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

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

szmarczak
Copy link
Member

@szmarczak szmarczak commented Apr 27, 2020 •
edited

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

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

targos
targos reviewed Apr 27, 2020
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

Please add something to the documentation

@szmarczak szmarczak changed the title dns: add resolver.cancel() dns: add a cancel() method to the promise DNS resolver Apr 27, 2020
@szmarczak
Copy link
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
rickyes reviewed Apr 27, 2020
View reviewed changes
test/parallel/test-dns-promise-channel-cancel.js Outdated
@@ -0,0 +1,33 @@
'use strict';
Copy link
Contributor

@rickyes rickyes Apr 27, 2020 •
edited

Choose a reason for hiding this comment

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

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

@szmarczak 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
rickyes reviewed Apr 27, 2020
View reviewed changes
doc/api/dns.md Outdated Show resolved Hide resolved
@szmarczak szmarczak requested a review from jasnell April 27, 2020 19:22
@addaleax addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 29, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@szmarczak
Copy link
Member Author

This is good to go... I guess?

@addaleax
Copy link
Member

addaleax commented May 9, 2020

@szmarczak Yes, once CI is green 👍

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2020
@targos
Copy link
Member

targos commented May 10, 2020

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
Copy link
Member

Ping @szmarczak

@szmarczak
Copy link
Member Author

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

@aduh95
Copy link
Contributor

aduh95 commented Nov 9, 2020

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

@aduh95 aduh95 added author ready PRs that have at least one approval, no pending requests for changes, 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 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
Copy link
Collaborator

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

@szmarczak
Copy link
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
Copy link
Member Author

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

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

This comment has been minimized.

Sign in to view
@szmarczak
Copy link
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
Copy link
Collaborator

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

@aduh95
Copy link
Contributor

aduh95 commented Nov 14, 2020

Landed in 9ea0fae

@aduh95 aduh95 closed this Nov 14, 2020
aduh95 pushed a commit that referenced this pull request Nov 14, 2020
@szmarczak @aduh95
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
@szmarczak @codebytere
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
@codebytere
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 codebytere mentioned this pull request Nov 22, 2020
Merged
codebytere added a commit that referenced this pull request Nov 24, 2020
@codebytere
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
@codebytere
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
@codebytere
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
@szmarczak @targos
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 danielleadams mentioned this pull request May 3, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@rickyes rickyes rickyes left review comments

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

@aduh95 aduh95 aduh95 approved these changes

@juanarbol juanarbol juanarbol 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. 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
@szmarczak @addaleax @nodejs-github-bot @targos @BridgeAR @aduh95 @jasnell @cjihrig @juanarbol @rickyes