nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.65k stars 29.08k forks source link

deprecate domains #66

Closed jonathanong closed 9 years ago

jonathanong commented 9 years ago

what's the status on domains? can we deprecate it before v1?

juliangruber commented 9 years ago

afaik no breaking changes will happen until v1

jonathanong commented 9 years ago

just labeling something as deprecated is not a breaking change

juliangruber commented 9 years ago

gotcha, :+1: from me, domains have only meant pain to me personally

rvagg commented 9 years ago

I'm on board, I bet a bunch of TC members would be too, marking as tc-agenda

indutny commented 9 years ago

+1

Marak commented 9 years ago

I never understood why domains were added to core.

It seemed wrong at the time, and still seems equally as wrong.

For backwards compatibility, will it be possible to put this functionality into an npm module as a custom add-on? Does this already exist?

Fishrock123 commented 9 years ago

+1 domains are confusing and I hear nothing but incompatibilities and edge cases

othiym23 commented 9 years ago

+1 sad :trumpet:

@Marak

I never understood why domains were added to core.

There's a place for unified sync & async error handling for Node, and due to the way that state propagates through core, there at least need to be hooks exposed by core to facilitate this. uncaughtException is too gross-grained, not everybody uses (or likes) promises, try/catch + stream error listeners + callback convention is complicated to teach and learn and harder to master, etc. Probably the best effort at this so far is @piscisaureus and co's zones, but that solution has limitations that are going to be tough to resolve outside core.

I'm hopeful that @trevnorris's AsyncWrap can provide the foundation for a better / lower-impact way of building something useful along these lines in the future, and I consider domains a very useful failure that helped point to what AsyncWrap / AsyncListener should do.

bnoordhuis commented 9 years ago

It would have to go through the TC but I don't think there is much opposition to deprecating domains. Who volunteers to follow up with a pull request?

tellnes commented 9 years ago

@bnoordhuis A deprecation warning if you call domain.create()? Or when you require it since it has side effects (acording to code comments).

bnoordhuis commented 9 years ago

@tellnes I'll leave that to your own judgment. Doing it at require() time would get the message out most effectively but it might end up being extremely annoying.

If you do do it at require() time, you should also update lib/repl.js and make it load the domain module lazily.

tellnes commented 9 years ago

lib/repl.js needs to be changed anyway because it is calling domain.create().

rlidwka commented 9 years ago

Is there a suitable replacement that could be used? Repl is a valid use-case for example.

tellnes commented 9 years ago

@rlidwka Yes, the new tracing module in core. Please see my implementation in #75.

jodytate commented 9 years ago

+1 what @Marak said and +1 @Fishrock123 said, so +2

jmcbee commented 9 years ago

What is TC? Only TechCrunch is going in my mind.

SomeoneWeird commented 9 years ago

See Technical Committee

benjamingr commented 9 years ago

+1 domains have only brought me pain and suffering in my code and were not good enough in practice. I'd love to see them removed in v1 and deprecated today.

scottnonnenberg commented 9 years ago

If domain isn't the right approach, I'd love to know the standard recommendation for clean error handling in http servers. At minimum we need to be able to:

  1. Return an error as a response to the original request that generated it
  2. Shut down gracefully (no socket hang-ups), allowing time to finish other in-process async operations

Both of these are already implemented via domain in my library thehelp-cluster. As far as I can tell the only other way to do number 1 is via comprehensive Promises or try/catch wrapping. Number 2 is easier; it and other process-level stuff can can be done via process.uncaughtException. Is there something I'm missing?

trevnorris commented 9 years ago

I'm not sure I'd recommend doing a full deprecation until proper exception handling is added to AsyncWrap. The latest patch introduced just what was needed to get v0.12 out the door. Still need to add JS support for timers/nextTick, change the exception flow control to use the custom error callback and acknowledge the fact we won't be able to touch exceptions that occur in Promises.

rvagg commented 9 years ago

TC today agreed to a "soft deprecation" of domains, i.e. a documentation change rather than a full deprecation for now. Someone will accept #15 but it'll effectively be the last major addition to domains from here. Concerns were mainly around the lack of a solid alternative to domains for now but mostly the TC is +1 on moving to an eventual removal of domains, which would have a proper deprecation in the path somewhere. See TC minutes in the docs directory for more information, merged in #144

benjamingr commented 9 years ago

+1 awesome news

rvagg commented 9 years ago

action being taken in #141, please comment there or open a new issue if you have anything further to add.

scottnonnenberg commented 9 years ago

@benjamingr I would love to hear more about your negative experiences with domains. As @rvagg said on #141 - "If you do a google search for "node.js domains" the results are almost all positive."

naholyr commented 9 years ago

:+1: @scottnonnenberg Never encountered any issue with domains. Never did some complex gymnastic with them anyway, but the API is very simple and straightforward: domain.create().run(foo), grab any error in an isolated "error" event. Good for me.

I don't get how we can deprecate something that covers a use case that cannot be covered otherwise, without providing clear (and ready) alternatives.

algesten commented 9 years ago

@scottnonnenberg @naholyr The problem isn't the user experience of domains, as you say the API is simple and users are generally happy with it, but the consequences domains have in maintenance and complications for the node.js core code – that seemingly simple API impacts all of the code base in a too negative way.

Domains in its current form must go. That's certain. What the alternative is, no one knows.

I don't get how we can deprecate something that covers a use case that cannot be covered otherwise, without providing clear (and ready) alternatives.

If we flip that the other way around. We know domains must go, and will have to be removed at some point soon-ish. Would you prefer there be no indication of this?

naholyr commented 9 years ago

If we flip that the other way around. We know domains must go, and will have to be removed at some point soon-ish. Would you prefer there be no indication of this?

Well it's more acceptable with the upper explanation.

Anyway, I really feel like it should go only when there is an immediate gain then, or at lease a clear alternative. Like "look, as we removed domains, we could easily implement this long-time feature, go get it :D", or even better like "we removed domains, here is the equivalent code for your usual use case". Or users feel the loss without any counterpart, and it will greatly impact release adoption.

algesten commented 9 years ago

@naholyr

Yes, this is exactly how the TC was talking about it and why it was called a "soft deprecation". At this point it's only a heads up to users that this API will be changing. However everyone is entirely clear on that domains fill an important role and requires some (maintainable) alternative.

roberttaylortech commented 8 years ago

Going to do my best to link together a few different ideas, issues and and some notes. I definitely don't have a full handle on all the moving parts, but I have plenty of experience trying to patch around the issues and resolve my own countless hours of pain. :)

My initial issue wasn't related to Bluebird it was with session middleware - totally awful for an entire web app to fall down from that. So we switched to domain.active based on that note from the Bluebird issue and thought we were in good shape. However, next we ran up against: https://github.com/nodejs/node-v0.x-archive/issues/8648 where anything using native Promises similarly lost context. We went ahead and dealt with out first major issue with native Promises that appeared in co with a little patch (https://github.com/tj/co/issues/239). Seemed to work well - though maybe instead of that patch we could have switched to Bluebird for our co-routine execution and been solved? (Though with all these issues floating around, it's not exactly clear that would work)

All seemed well -- then we realized that in fact session middleware was losing context with domain.active just the same as it was with CLS ( is session middleware also using native Promises and suffering the same fate?) . We ended up tinkering for hours and believe we finally found a way to have the rest of the execution re-bound to the domain -- see my comment on this issue: https://github.com/othiym23/node-continuation-local-storage/issues/29

I think its also noteworthy on the side (for those who haven't read the code of CLS) that CLS appears to globally monkey patch native promises (just to make aware another place things can fall apart.

Aside from tying together a bunch of related needs/issues, again # 1 is that whether it be CLS or domain.active or similar, it seems much needed (and definitely very much for product I am working on) that something dependable for asynchronous context emerge. Lots of moving parts, I know!

terinjokes commented 8 years ago

I'm also running into the use cases where domains or CLS would prove useful, but after 11 months of "soft" deprecation, I'm we've still not been given any heads up on what to replace the use cases with.

Is this a problem still being worked on?

Fishrock123 commented 8 years ago

@terinjokes Not really, it's pretty hard and most of us already have our hands full.

terinjokes commented 8 years ago

@Fishrock123 no problem, mostly wanted to check in.

bjouhier commented 8 years ago

@terinjokes We (Sage) have some experience in this area, as we have been programming node.js with CLS and structured exceptions for almost 5 years.

We use CLS in our product to propagate our security and localization context.

We use structured exception handling because it gives us robust code: code where exceptions are always caught and handled in context (try/catch) and where we can guarantee (and easily verify) that resources are always released and program invariants always restored (try/finally). To be contrasted with code where exceptions end up in a global (context-less) uncaught exception handler or crash the whole process.

We do all our parallelization with futures (aka thunks). This blends very smoothly with structured exceptions. The important rule is that every execution thread should either be executed in fire and forget mode or joined with a parent thread (of course these threads are green). In the fire and forget case, every child thread is responsible for its own error handling; this happens naturally in the callback plug that terminates the child. In the joined case, any exception thrown by a child thread is propagated to its parent thread at the join point; it can then be handled in context in the parent.

None of this is really hard to implement as long as you accept to reify continuations in the language. This is what streamline.js does with its _ and what ES7 is going to do with async/await. CLS was not hard to implement in streamline.js because the code is preprocessed. With ES7 async/await it could be implemented by monkey patching promises (less obvious because promises have more surface API).

If you don't reify continuations (in the language or through special programming discipline), you will be struggling. There is no free lunch.

My 2 cents.

nmccready commented 8 years ago

Is continuation-local-storage not a viable alternative? It was listed here and seems to work quite well.

mfuhrmeister commented 8 years ago

@nmccready

As @souluniversal says... unfortunately NO!! I could not find any suitable solution for the issues mentioned before.

EDIT: Zone seems to be (even though under development) promising!?

Fishrock123 commented 8 years ago

@mfuhrmeister Zone looks like it was last updated a year ago, so it might be abandoned.


Most people haven given up on replacing/using domains at this point. Generally it's an unreasonable amount of work. Anything like this ties so deeply into everything it is almost impossible to maintain well. Also coming up with the correct API and error handling heuristics is extremely difficult.

Perhaps there will be an alternative in some future, but don't count on it. Please do not use domains unless you absolutely need them.

cybrown commented 8 years ago

Are domains going to be removed before any other solution like continuation local storage is available ?

trevnorris commented 8 years ago

No

dman777 commented 8 years ago

I wish they did not do away with Domains, or at least deprecate it with a like replacement. I prefer Domains myself, the api was very simple and I liked the error reporting use of it.

emilsedgh commented 8 years ago

Heartbreaking and surprising!

Domains are very useful for me. I would like to describe my usecase and hear your ideas about how it could be handled otherwise.

I'm maintaing an API and I want each endpoint to be atomic. That means, if an endpoint does multiple things and for any reason, fails one of them, I want all the changes to be reverted.

The way I handle this situation is using domains. For each request, a new domain is created. A new database connection is assigned to the domain. If the request fails, the database connection ROLLBACK's and an error is sent to the user.

This is super useful and I would like to hear how other people handle this scenario.

The following gist is a code dump directly from our project but you get the idea: https://gist.github.com/emilsedgh/a01a04b81846d7ae09dc

bjouhier commented 8 years ago

@emilsedgh Here is how we do it:

helper.withConnection = function(_, body) {
  var connection = pool.allocConnection(_, transacted);
  try {
    body(_, connection);
    if (transacted) connection.commit(_);
  } catch (ex) {
    logException(ex);
    if (transacted) connection.rollback(_);
  } finally {
    pool.releaseConnection(connection);
  }
}

// everywhere else
helper.withConnection(_, function(_, connection) {
  // do the job
});

We are writing our code in sync style with streamline.js but you could use other solutions: fibers, suspend, co, genny, ES7 async/await (with babel).

Domains is an experimental API. It gives you dynamic scopes but it has fundamental problems with dynamic nested sequences: enter D1, enter SUBD2, exit D1, exit SUBD2.

The combination of sync-style code and classical SEH (structured exception handling) is a simple and robust solution. Scopes are static and easy to analyze.

emilsedgh commented 8 years ago

@bjouhier Problem with that approach is that you will have to carry the connection around.

Models are usually something like this:

User.getById(user_id, cb);

I do not want to make something like this

User.getById(user_id, connection, cb);

How can I achieve this using any other solution?

My models are currently abstract from routes and express. They are not aware of requests at all. All they do is to pick up connections from process.domain.db

This also allows me to implement other transaction policies. For example in my cli scripts where they use the same models, there is no concept of request. Therefore, my cli scripts provide a database connection on process.domain.db without the notion of transactions.

Domains allow me to have this whole thing neatly abstracted away.

I don't want to keep talking about my very specific use case. Just trying to point out that Domains are useful to some of us.

bjouhier commented 8 years ago

Problem with that approach is that you will have to carry the connection around.

@emilsedgh Not really. Streamline.js provides CLS (Continuation Local Storage). See https://github.com/Sage/streamline-runtime/blob/master/index.md. We don't use it for db connections but we use it extensively for locale and security context.

fibers also provides CLS, via Fiber.current.

I don't know about other libraries (suspend, co, genny).

aaaristo commented 8 years ago

It looks to me like ES6 generators have the potential to solve both error handling and context propagation issues. In fact one could just use try/catch for error handling and use the root function of the call stack as the scope for the context propagation. Here is an ugly gist:

https://gist.github.com/aaaristo/85415d4581ac6d719a5a

There the run function has a context variable that can be accessed by any generator function deep in the call stack (by calling the run.withContext generator), since essentially any generator function in the call stack is implicitly talking to the root function, to have its yield(s) resolved. I found this presentation much useful: https://youtu.be/DqMFX91ToLw?t=23m29s.

DaSpawn commented 8 years ago

I have recently ran into an issue with a module that would kill my express process before it could reply with an error message. I tried promises, try/catch, etc and nothing could catch the error where I could reply to the connection with an actual error message (I had to resort to secondary web sockets to carry back the error, sometimes). I have been trying to find a solution to this for years as my request handlers have no clue what socket they came in on, and the socket can not be easily passed to child processes

I found domains the other day, appear to be the greatest things since sliced bread, I am able to catch any error that happens within request processing and always reply with something, error or not, before I exit process (even all the error event catchers I had to litter add all over the place could not catch them all). When a browser returns a blank page the site is 100% broken to the end user, no matter what the problem really is.

and then I read domains are being depreciated, I was hoping to integrate into everything, that way all errors are isolated and easily identifiable (already shortened a module by half just eliminating all the try catch madness all over the place)

What is the alternative to domains that will allow me to catch an uncaughtException BEFORE it hits the main process? My exception handlers at the top should never be reached, they have no idea about the connections, and the fatal error is usually something pointless (like connection abort, bad file parsing, etc). My uncaughtException handler is going to have to become a huge beast just to handle various error handling from the different areas of the application

I found it incredibly easier to just throw whenever there was a problem anywhere, I knew the function would exit right there, but there is no way to catch a throw before hitting main process handler. Domains allowed me to catch any error, even if I threw them myself, and always return a response of some kind

Am I missing something here? what is the problem with domains? Why are they being depreciated without any real simple alternatives?

ofrobots commented 8 years ago

@DaSpawn There are a bunch of reasons why domains were deprecated. While there doesn't exist a definitive post-mortem on what all is wrong with domains, here is a list of things that didn't work quite well with domains: 1) domains didn't nest automatically, 2) subtle differences in sync vs. async error handling, 3) lack of proper resource finalization semantics, etc.

Having said that, domains are probably not going to be removed from core anytime soon. You can continue using them, just be aware of subtle semantic weirdnesses.

Node.js core recently landed AsyncWrap which contains enough low-level hooks that an alternative to domains to could be implemented on top. What should the user level API for this look like is an open problem. It is not even clear if async error-handling should be in core vs. the more minimal hooks that solve a more fundamental problem.

Some of us (at least me) think that Dart's zones provide good API and semantics at least for async context propagation and possibly for async error handling. Here's proposal on zones for JS.

This is far from decided at this point, but I expect some active work on this in the coming months.

DaSpawn commented 8 years ago

(Dart) zones look like a good idea, but zones should be as simple as domains if they are to replace them (ie using standard callbacks and not Promises)

Marak commented 8 years ago

Thought it over some more. Domains are still a bad idea.

You've got three choices instead of using domains:

  1. Write correct code that always continues with error callbacks
  2. Add a global exception handler to catch the uncaught error, fix it, remove global handler
  3. If you are really stuck having to run unreliable code, then push it to a worker process via child process spawn and monitor stdout and stderr and standard spawn events ( error / close / end ).

FYI, I maintain a service which allows users to run arbitrary JavaScript code anonymously ( http://hook.io ), so I am somewhat familiar with this topic of discussion.

Jeff-Lewis commented 8 years ago

@Marak I agree with your strategies listed as best practices for node development. Though Domains offer more than just isolated error handling. There are many use cases for maintaining proper context across async boundaries for which many have tried to use Domains to solve.

Both Domains and CLS have their issues and drawbacks maintaining context and the node community needs a solid solution for this.

See recent proposals and discussions from the Ecma International group.

Other related work:

Marak commented 8 years ago

Yes. I read this document and also this library.

Smells like the smart people have gone off into a corner again. Kinda like when peerDependencies hit npm.

All of this is over-complication.

In a functional language, you should be use to passing scopes from top to bottom, and you should be good at it. If you don't like the mental model of this type of programming, you should switch to something more imperative like Python or Ruby.

Once you introduce the idea of domains or CLS into your JavaScript stack, you've essentially removed a level of simplicity in favor for complexity. I'd argue that with proper functional encapsulation and application design, you should never need either of these solutions and your application will be much easier to reason about.