X Tutup
The Wayback Machine - https://web.archive.org/web/20220423141342/https://github.com/nodejs/node/pull/33099
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.

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 label Apr 27, 2020
@addaleax addaleax added the semver-minor label Apr 27, 2020
@addaleax
Copy link
Member

@addaleax 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 changes
Copy link
Member

@targos targos left a comment

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 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 changes
test/parallel/test-dns-promise-channel-cancel.js Outdated
@@ -0,0 +1,33 @@
'use strict';
Copy link
Member

@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 changes
doc/api/dns.md Outdated Show resolved Hide resolved
@szmarczak szmarczak requested a review from jasnell Apr 27, 2020
@addaleax addaleax added the author ready label Apr 29, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 29, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 30, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 1, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 7, 2020

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

@szmarczak
Copy link
Member Author

@szmarczak szmarczak commented May 8, 2020

This is good to go... I guess?

@addaleax
Copy link
Member

@addaleax addaleax commented May 9, 2020

@szmarczak Yes, once CI is green 👍

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 9, 2020

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

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented May 9, 2020

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

@targos targos removed the author ready label May 10, 2020
@targos
Copy link
Member

@targos 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 BridgeAR commented May 27, 2020

Ping @szmarczak

@szmarczak
Copy link
Member Author

@szmarczak 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 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 request-ci and removed stalled labels Nov 12, 2020
@github-actions github-actions bot removed the request-ci label Nov 12, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 12, 2020

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

@szmarczak
Copy link
Member Author

@szmarczak 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 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 label Nov 13, 2020
@github-actions github-actions bot removed the request-ci label Nov 13, 2020
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@szmarczak
Copy link
Member Author

@szmarczak 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
Contributor

@nodejs-github-bot nodejs-github-bot commented Nov 14, 2020

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

@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Nov 14, 2020

Landed in 9ea0fae

@aduh95 aduh95 closed this Nov 14, 2020
aduh95 pushed a commit that referenced this issue 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 issue 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 issue 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 issue 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 issue 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 issue 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 issue 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

@rickyes rickyes

@jasnell jasnell

@addaleax addaleax

@cjihrig cjihrig

@aduh95 aduh95

@juanarbol juanarbol

@nodejs/net net

Assignees
No one assigned
Labels
author ready dns semver-minor
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
X Tutup