domain: remove `.dispose()` by addaleax · Pull Request #15412 · nodejs/node

addaleax · 2017-09-14T16:06:23Z

domain.dispose() is generally considered an anti-pattern, has been runtime-deprecated for over 4 years, and is a part of the domain module that can not be emulated by async_hooks; so remove it.

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

Affected core subsystem(s)

domain

/cc @trevnorris

addaleax · 2017-09-14T16:06:35Z

@nodejs/tsc since this is semver-major

mcollina

LGTM

cjihrig

LGTM. I think this should have the added bonus of improving code coverage on lib/domain.js.

jasnell · 2017-09-14T18:03:25Z

doc/api/deprecations.md

Might be a good idea to expand on the description in here to explain why it was removed and why it is an anti-pattern. That doesn't have to be in this PR, but it would be worthwhile at some point.

I think we had a whole document explaining why and how domains were broken, including .dispose()? I can’t seem to find it anywhere in our repos, though…

heh.. there's something floating around somewhere but I cannot remember where.

The domains postmortem is at https://nodejs.org/en/docs/guides/domain-postmortem/

jasnell

LGTM with a CITGM run

addaleax · 2017-09-15T18:07:05Z

CITGM on master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/987/
CITGM this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/988/

trevnorris · 2017-09-15T21:30:53Z

lib/timers.js

hah. so disposal has never worked for setImmediate(). let's use that as a demonstration of how little it must have been used.

trevnorris

I have goosebumps. Thanks for doing this. LGTM

Could add this to the commit message, though is mostly just to add insult to injury :-P

runtime-deprecated for over 4 years, and is a part of the domain module
(which is also deprecated) that can not be emulated by async_hooks; so
remove it.

Fishrock123

LGTM - but I would like it if the domains postmortem was linked from the commit.

Perhaps link both the website link and the original commit of the postmortem from this repo

https://nodejs.org/en/docs/guides/domain-postmortem/
4a74fc9776d825115849997f4adacb46f4303494

`domain.dispose()` is generally considered an anti-pattern, has been runtime-deprecated for over 4 years, and is a part of the `domain` module that can not be emulated by `async_hooks`; so remove it. Ref: https://nodejs.org/en/docs/guides/domain-postmortem/ Ref: 4a74fc9

addaleax · 2017-09-19T17:25:42Z

@Fishrock123 done!

jasnell · 2017-09-20T08:21:33Z

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

BridgeAR · 2017-09-20T13:46:41Z

Landed in 602fd36

`domain.dispose()` is generally considered an anti-pattern, has been runtime-deprecated for over 4 years, and is a part of the `domain` module that can not be emulated by `async_hooks`; so remove it. Ref: https://nodejs.org/en/docs/guides/domain-postmortem/ Ref: 4a74fc9 PR-URL: #15412 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

PR-URL: #15510 Refs: #15412 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`domain.dispose()` is generally considered an anti-pattern, has been runtime-deprecated for over 4 years, and is a part of the `domain` module that can not be emulated by `async_hooks`; so remove it. Ref: https://nodejs.org/en/docs/guides/domain-postmortem/ Ref: 4a74fc9 PR-URL: nodejs/node#15412 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

PR-URL: nodejs/node#15510 Refs: nodejs/node#15412 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

`domain.dispose()` is generally considered an anti-pattern, has been runtime-deprecated for over 4 years, and is a part of the `domain` module that can not be emulated by `async_hooks`; so remove it. Ref: https://nodejs.org/en/docs/guides/domain-postmortem/ Ref: 4a74fc9 PR-URL: nodejs/node#15412 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

PR-URL: nodejs/node#15510 Refs: nodejs/node#15412 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#15510 Refs: nodejs#15412 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

* **async_hooks** * Older experimental `async_hooks` APIs have been removed [[`d731369b1d`](d731369b1d)] **(SEMVER-MAJOR)** [#14414](#14414) * **Errors** * Multiple built in modules have been migrated to use static error codes * **Domains** * The long deprecated `.dispose()` method has been removed [[`602fd36d95`](602fd36d95)] **(SEMVER-MAJOR)** [#15412](#15412) * **File system** * `fs.ReadStream` and `fs.WriteStream` now make use of `.destroy()` [[`e5c290bed9`](e5c290bed9)] **(SEMVER-MAJOR)** [#15407](#15407) * `fs` callbacks are now invoked with an undefined `this` context [[`2249234fee`](2249234fee)] **(SEMVER-MAJOR)** [#14645](#14645) * **HTTP** * Socket timeout is set when the socket connects [[`10be20a0e8`](10be20a0e8)] **(SEMVER-MAJOR)** [#8895](#8895) * A bug causing the request `error` event to fire twice has been fixed [[`620ba41694`](620ba41694)] **(SEMVER-MAJOR)** [#14659](#14659) * The `pipe` method on `OutgoingMessage` has been disabled [[`156549d8ff`](156549d8ff)] **(SEMVER-MAJOR)** [#14358](#14358) * **HTTP/2** * The `--expose-http2` command-line argument is no longer required [[`f55ee6e24a`](f55ee6e24a)] **(SEMVER-MAJOR)** [#15535](#15535) * **Internationalization** * The `Intl.v8BreakIterator` class has been removed [[`668ad44922`](668ad44922)] **(SEMVER-MAJOR)** [#15238](#15238) * **OS** * `os.EOL` is now read-only [[`f6caeb9526`](f6caeb9526)] **(SEMVER-MAJOR)** [#14622](#14622) * **Process** * It is now possible to pass additional flags to `dlopen` [[`5f22375922`](5f22375922)] **(SEMVER-MAJOR)** [#12794](#12794) * **Timers** * Using a timeout duration larger than 32-bits will now emit a warning [[`ce3586da31`](ce3586da31)] **(SEMVER-MAJOR)** [#15627](#15627) * **TLS** * `parseCertString` has been deprecated [[`468110b327`](468110b327)] **(SEMVER-MAJOR)** [#14249](#14249) * Type-checking for `key`, `cert`, and `ca` options has been added [[`a7dccd040d`](a7dccd040d)] **(SEMVER-MAJOR)** [#14807](#14807)

addaleax added domain Issues and PRs related to the domain subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Sep 14, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. domain Issues and PRs related to the domain subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. labels Sep 14, 2017

addaleax removed the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 14, 2017

mcollina approved these changes Sep 14, 2017

View reviewed changes

BridgeAR approved these changes Sep 14, 2017

View reviewed changes

cjihrig approved these changes Sep 14, 2017

View reviewed changes

jasnell reviewed Sep 14, 2017

View reviewed changes

jasnell approved these changes Sep 14, 2017

View reviewed changes

trevnorris reviewed Sep 15, 2017

View reviewed changes

trevnorris approved these changes Sep 15, 2017

View reviewed changes

fhinkel approved these changes Sep 16, 2017

View reviewed changes

Fishrock123 approved these changes Sep 19, 2017

View reviewed changes

addaleax force-pushed the rm-dispose branch from d5e2793 to 77da410 Compare September 19, 2017 17:25

BridgeAR closed this Sep 20, 2017

vsemozhetbyt mentioned this pull request Sep 20, 2017

doc: delete link to removed doc part #15510

Closed

2 tasks

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

Uh oh!

Conversation

addaleax commented Sep 14, 2017

Checklist

Affected core subsystem(s)

Uh oh!

addaleax commented Sep 14, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

Fishrock123 Sep 14, 2017

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 15, 2017

Uh oh!

trevnorris Sep 15, 2017

Choose a reason for hiding this comment

Uh oh!

trevnorris left a comment

Choose a reason for hiding this comment

Uh oh!

Fishrock123 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 19, 2017

Uh oh!

jasnell commented Sep 20, 2017

Uh oh!

BridgeAR commented Sep 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

Fishrock123 left a comment •

edited

Loading