Skip to content

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

Closed
szmarczak wants to merge 3 commits intonodejs:masterfrom
szmarczak:resolver-cancel
Closed

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

Conversation

@szmarczak
Copy link
Member

@szmarczak szmarczak commented Apr 27, 2020 •
edited
Loading

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

addaleax commented Apr 27, 2020

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

szmarczak commented Apr 27, 2020

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

@rickyes rickyes Apr 27, 2020 •
edited
Loading

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.

jasnell
jasnell reviewed Apr 27, 2020
View reviewed changes
@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
@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

nodejs-github-bot commented Apr 29, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 30, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 1, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 7, 2020

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

@szmarczak
Copy link
Member Author

szmarczak commented May 8, 2020

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

nodejs-github-bot commented May 9, 2020

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 9, 2020

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

BridgeAR commented May 27, 2020

Ping @szmarczak

@szmarczak
Copy link
Member Author

szmarczak commented May 28, 2020

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

nodejs-github-bot commented Nov 12, 2020

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

@szmarczak
Copy link
Member Author

szmarczak commented Nov 13, 2020

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

szmarczak commented Nov 13, 2020

@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

szmarczak commented Nov 14, 2020

@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

nodejs-github-bot commented Nov 14, 2020

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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Juan José Arboleda <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
@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

@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

+1 more reviewer

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