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

doc: add domain post mortem #6159

Merged
merged 1 commit into from
Apr 15, 2016
Merged

Conversation

trevnorris
Copy link
Contributor

Checklist
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

doc

Description of change

The domain module did not live a long and fruitful life. Instead it was
ultimately deprecated and abandon. The domain postmortem addresses why
this happened so that it doesn't happen again in the future.

Also included is an example script that accompanies the postmortem
analysis.

R=@Fishrock123
R=@nodejs/documentation

@trevnorris trevnorris added doc Issues and PRs related to the documentations. domain Issues and PRs related to the domain subsystem. labels Apr 11, 2016
built-in http benchmark, `http_simple.js`, without domains it can handle over
22,000 requests/second. Whereas if it's run with `NODE_USE_DOMAINS=1` that
number drops down to under 17,000 requests/second. In this case there is only
as single global domain. If we edit the benchmark so the http request callback
Copy link
Contributor

Choose a reason for hiding this comment

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

as -> a

Copy link
Contributor

Choose a reason for hiding this comment

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

a single domain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

global is more technically correct. though even more so if it said a single active global domain. since the domain is activated at startup and left active for the remainder of the script.

@eljefedelrodeodeljefe
Copy link
Contributor

docs-wise LGTM with nits. Good write-up.

intercept the exceptions of unrelated code in a different module. Preventing
the originator of the code from knowing about its own exceptions.

Another is the how domains route errors automatically if no `'error'` handler
Copy link
Contributor

Choose a reason for hiding this comment

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

Starting with "Another" sounds kind of funny here. Maybe change it to "Domains also route errors..."

Copy link
Member

Choose a reason for hiding this comment

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

Perhaps "Another issue is that domains route errors automatically...."

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2016

While on the topic, are there any thoughts about replacements and/or the way forward? Would it be beneficial to include at least a little information on that at the end of this document or no?

@trevnorris
Copy link
Contributor Author

@mscdex I'm hopeful that AsyncWrap can assume the functionality of continuous local storage. The postmortem intended to demonstrate the difficulty in properly implementing asynchronous error handling, and how anything mildly complex leads to a situation where reliable resource cleanup is impossible.

Since my initial inception of AsyncListener, and its error handling implementation, I've reiterated on how to make this more feasible. After the initial features are implemented I'll write up a design doc for review.


As we can see from this example a lot more must be done to properly clean up
resources when something fails than what can be done strictly through the
domain API. All domains offer us is an exception aggregation mechanism. Even

Choose a reason for hiding this comment

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

All domains offer us looks a bit weird. @trevnorris Is this the right wording?

@Knighton910
Copy link

LGTM ✅

@trevnorris
Copy link
Contributor Author

All nits addressed.

@mscdex Would you like a small section about how we plan on moving forward now that this functionality has been deprecated? Would also like input from others on how this should be addressed.

`domain.enter()`. Which then acts as a catch-all for any exception in the
future that couldn't be observed by the thrower. Allowing a module author to
intercept the exceptions of unrelated code in a different module. Preventing
the originator of the code from knowing about its own exceptions.
Copy link
Member

Choose a reason for hiding this comment

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

A code sample here would do wonders to explain this to people considering domains.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added.

@benjamingr
Copy link
Member

I left some comments @trevnorris - otherwise excellently written 👏

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2016

@trevnorris It's up to you, but after reading all of it it just seemed to kind of leave me hanging at the end.

@jasnell
Copy link
Member

jasnell commented Apr 12, 2016

LGTM

@trevnorris
Copy link
Contributor Author

@benjamingr Added two examples, changed error message and added a new section, with example, on using domain for data propagation.

@mscdex See your point. I'll try to get something written up, but no promises.

NOTE: Been working on this too long and too late. I don't take corrections to my grammar personally, so feel free to request changes. I know there are improvements that can be made, but too tired to see them.

@benjamingr
Copy link
Member

Very nice.

@Fishrock123
Copy link
Contributor

I think domain-resource-cleanup.js should be domain-resource-cleanup-example.js

Or examples/domain-resource-cleanup.js, though I think domain-resource-cleanup-example.js should be fine.

@Fishrock123
Copy link
Contributor

LGTM otherwise.


### API Gaps

While APIs based using `EventEmitter` can use `bind()` and errback style
Copy link
Member

Choose a reason for hiding this comment

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

"based using" -> "based on using"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed.

@rvagg
Copy link
Member

rvagg commented Apr 14, 2016

Nice work @trevnorris, so good to have all of this in one place that we can point to when it comes up.

I think this needs an intro to explain why this doc exists in the first place, something along the lines of "The Domains API has been deprecated, this document explains some of the major problems observed with Domains and why the decision was made to scrap, rather than attempt to fix the API".

Another possible addition here could be a section on complexity and the overhead that Domains has introduced in maintenance of so many parts of core.

/cc @misterdjules who may have some good insight to add on this topic.

Even manually removing the connection via `d.remove(c)` does not prevent the
connection's error from being automatically intercepted.

A failure that plagues both error routing and exception handling are the
Copy link
Member

Choose a reason for hiding this comment

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

Grammar nit: "A failure ... are ..." <-- singular subject, plural verb.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@trevnorris
Copy link
Contributor Author

@mscdex I've added a "Looking Ahead" section at the end. Hopefully that's along the lines of what you were looking for.

@rvagg I would be happy to add a prologue or such if you have more details of what you're looking for.

@mscdex
Copy link
Contributor

mscdex commented Apr 15, 2016

@trevnorris LGTM

Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: nodejs#6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
@trevnorris trevnorris closed this Apr 15, 2016
@trevnorris trevnorris deleted the domain-postmortem branch April 15, 2016 21:13
@trevnorris trevnorris merged commit 4a74fc9 into nodejs:master Apr 15, 2016
@trevnorris
Copy link
Contributor Author

Thanks all. Landed in 4a74fc9.

MylesBorins pushed a commit that referenced this pull request Apr 19, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: #6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: #6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: #6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 20, 2016
MylesBorins pushed a commit that referenced this pull request Apr 20, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: #6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
@MylesBorins
Copy link
Contributor

@trevnorris I've gone ahead and marked this don't land on v4

Please let me know if you think it should be backported to lts docs

MylesBorins pushed a commit that referenced this pull request Apr 21, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: #6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
joelostrowski pushed a commit to joelostrowski/node that referenced this pull request Apr 25, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: nodejs#6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
jasnell pushed a commit that referenced this pull request Apr 26, 2016
Do to various reasons, outlined in the committed document, domains were
only in core for 2 years before being deprecated. This outline explains
why they received criticism from the community and never gained traction
with module authors.

Also included is an example script that accompanies the postmortem
analysis.

PR-URL: #6159
Reviewed-By: Robert Lindstädt <[email protected]>
Reviewed-By: Kelvin Knighton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Brian White <[email protected]>
@trevnorris
Copy link
Contributor Author

@thealphanerd Sounds fine. This is only informative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. domain Issues and PRs related to the domain subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.