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

domain: remove .dispose() #15412

Closed
wants to merge 1 commit into from
Closed

domain: remove .dispose() #15412

wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

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

@nodejs/tsc since this is semver-major

@addaleax addaleax removed the timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. label Sep 14, 2017
mcollina
mcollina approved these changes Sep 14, 2017
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

cjihrig
cjihrig approved these changes Sep 14, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

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

jasnell
jasnell reviewed Sep 14, 2017
View reviewed changes
doc/api/deprecations.md

[`Domain.dispose()`][] is deprecated. Recover from failed I/O actions
[`Domain.dispose()`][] is removed. Recover from failed I/O actions
explicitly via error event handlers set on the domain instead.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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…

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

jasnell
jasnell approved these changes Sep 14, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with a CITGM run

@addaleax
Copy link
Member Author

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
trevnorris reviewed Sep 15, 2017
View reviewed changes
lib/timers.js
//
// https://github.com/nodejs/node-v0.x-archive/issues/2631
if (domain._disposed)
continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

trevnorris
trevnorris approved these changes Sep 15, 2017
View reviewed changes
Copy link
Contributor

@trevnorris trevnorris left a comment

Choose a reason for hiding this comment

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

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
Fishrock123 approved these changes Sep 19, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

@addaleax
domain: remove .dispose()
77da410 
`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
Copy link
Member Author

@Fishrock123 done!

@jasnell
Copy link
Member

jasnell commented Sep 20, 2017

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

@BridgeAR
Copy link
Member

Landed in 602fd36

@BridgeAR BridgeAR closed this Sep 20, 2017
BridgeAR pushed a commit that referenced this pull request Sep 20, 2017
@addaleax @BridgeAR
domain: remove .dispose()
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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@vsemozhetbyt vsemozhetbyt mentioned this pull request Sep 20, 2017
Closed
2 tasks
vsemozhetbyt added a commit that referenced this pull request Sep 21, 2017
@vsemozhetbyt
doc: delete link to removed doc part
cd1b55a 
PR-URL: #15510
Refs: #15412
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@addaleax
domain: remove .dispose()
316c95e 
`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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@vsemozhetbyt
doc: delete link to removed doc part
3e451bc 
PR-URL: nodejs/node#15510
Refs: nodejs/node#15412
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@addaleax
domain: remove .dispose()
7dfcd94 
`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 <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trevor Norris <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
@vsemozhetbyt
doc: delete link to removed doc part
c02f39d 
PR-URL: nodejs/node#15510
Refs: nodejs/node#15412
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
lukaszewczak pushed a commit to lukaszewczak/node that referenced this pull request Sep 23, 2017
@vsemozhetbyt @lukaszewczak
doc: delete link to removed doc part
11d72f2 
PR-URL: nodejs#15510
Refs: nodejs#15412
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell added a commit that referenced this pull request Oct 17, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
6891ac2 
* **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)
jasnell added a commit that referenced this pull request Oct 29, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
7165af5 
* **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)
jasnell added a commit that referenced this pull request Oct 29, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
a618552 
* **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)
jasnell added a commit that referenced this pull request Oct 29, 2017
@jasnell
2017-10-31, Version 9.0.0 (Current)
8d28704 
* **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)
@jasnell jasnell mentioned this pull request Oct 30, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@Fishrock123 Fishrock123 Fishrock123 approved these changes

@trevnorris trevnorris trevnorris approved these changes

@fhinkel fhinkel fhinkel approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@addaleax @jasnell @BridgeAR @mcollina @fhinkel @trevnorris @Fishrock123 @cjihrig @nodejs-github-bot