Domains3 #3109
Domains3 #3109
Conversation
|
example: Please imagine that |
|
@koichik No, in the B) section of your example, it would still be part of the domain. ReqWraps are bound to the domain that is active at the time of their creation, and so ReqWrap callbacks are run in the context of the domain. |
|
Also, fs.readFile uses an EventEmitter internally, so it would also be implicitly bound to the active domain at the time of its creation. |
|
@isaacs - Sorry, this is my mistake. I cannot reproduce what I wrote in the previous comment. |
|
I understood my failure. First time, I used function foo(path, cb) {
// here is a part of domain
process.nextTick(function() {
// here is not a part of domain
throw new Error(); // uncaught by domain
});
}After changing |
|
Ah! Yes! It seems that nextTick is a hole. I'll look into that. Nice find! |
|
Er, wait, no, it isn't. I spoke too soon. nextTick still goes through the same entry point. Updated the gist: https://gist.github.com/2385382 |
|
No, I didn't speak too soon :) It's only working by accident. There is indeed a possibility for escape here. I'll fix it soon. |
|
@isaacs - Confirmed, the gist works well. However, my code does not work. var domain = require('domain');
var d = domain.create();
d.on('error', function(err) {
console.log('Domain caught error', err);
});
d.run(function() {
foo();
});
function foo() {
// here is a part of domain
process.nextTick(function() {
// here is not a part of domain
throw new Error('nexttick in foo');
});
}result: In this case, there is no active domain within |
|
I was too late :) |
|
This would close the leak: diff --git a/src/node.js b/src/node.js
index 3322df6..d0ea1b4 100644
--- a/src/node.js
+++ b/src/node.js
@@ -249,6 +249,7 @@
};
process.nextTick = function(callback) {
+ if (process.domain) callback = process.domain.bind(callback);
nextTickQueue.push(callback);
process._needTickCallback();
};However, the cleaner approach would be to push objects rather than functions into the tick queue, and assign the active domain to them. Also, it would be nice if the MakeCallback function could check the _disposed member of the relevant domain, and not fire the callback at all in that case. |
|
c9c44e0 fixes the issue that @koichik found. 7232710008bdfabd17b6328e755dcf6d4b745929 seems somewhat controversial to me. It prevents callbacks from firing at all if they are bound to a domain that has been disposed. I could see this leading in some rare cases to memory leaks, since there may be IO that needs to be cleaned up. On the other hand, it's the responsibility of the user to clean up their stuff when they call domain.dispose(). |
|
It seems that |
|
@isaacs Confirmed that setTimeout was fixed. Thanks. But a callback of setInterval() is still out of domain. Fixed in 317f990 as below. Please review it. |
|
@shigeki Indeed. I fixed this on c2e4a5dc7530f77a16cfe97793c41e64770630bf by making the key always "domain" rather than "_domain", so that MakeCallback picks it up properly. |
|
That's much more elegant. Confirmed that timers works good. |
There was a problem hiding this comment.
Is swallowing errors a good idea here? Shouldn't they be logged or emitted to somewhere somehow?
There was a problem hiding this comment.
Well, they'd always be errors like that you can't .end() a stream that's already closed or something. Ie, useless and extraneous. Until we have a consistent way to shut things down, and to tell if they've been shut down, I think this is about the best we can do.
This is a squashed commit of the main work done on the domains-wip branch.
The original commit messages are preserved for posterity:
* Implicitly add EventEmitters to active domain
* Implicitly add timers to active domain
* domain: add members, remove ctor cb
* Don't hijack bound callbacks for Domain error events
* Add dispose method
* Add domain.remove(ee) method
* A test of multiple domains in process at once
* Put the active domain on the process object
* Only intercept error arg if explicitly requested
* Typo
* Don't auto-add new domains to the current domain
While an automatic parent/child relationship is sort of neat,
and leads to some nice error-bubbling characteristics, it also
results in keeping a reference to every EE and timer created,
unless domains are explicitly disposed of.
* Explicitly adding one domain to another is still fine, of course.
* Don't allow circular domain->domain memberships
* Disposing of a domain removes it from its parent
* Domain disposal turns functions into no-ops
* More documentation of domains
* More thorough dispose() semantics
* An example using domains in an HTTP server
* Don't handle errors on a disposed domain
* Need to push, even if the same domain is entered multiple times
* Array.push is too slow for the EE Ctor
* lint domain
* domain: docs
* Also call abort and destroySoon to clean up event emitters
* domain: Wrap destroy methods in a try/catch
* Attach tick callbacks to active domain
* domain: Only implicitly bind timers, not explicitly
* domain: Don't fire timers when disposed.
* domain: Simplify naming so that MakeCallback works on Timers
* Add setInterval and nextTick to domain test
* domain: Make stack private
Submitted for your review.
See the included tests and docs for examples of usage. Most of the time, the user doesn't have to do anything special to take advantage of them: all callbacks that are assigned as part of a
ReqWrapor timer are automatically attached, as is anything called byEventEmitter.prototype.emit.To enable domains, the domain module must be loaded at least once.
require('domain')sets the flag on the events module to tell it to register new EventEmitters with the active domain. All the other behavior is based on the presence of an active domain atprocess.domain.A domain is made "active" by being entered. If a timer or EventEmitter is explicitly added to a domain (or implicitly added by being created while a domain is active), then that domain will be activated for the callback resulting from that timer or EventEmitter. Same for ReqWraps, however since they are not exposed to the user, there is no way to explicitly add them; they can only be implicitly bound to a domain.
The implementation works by hooking into the
process.on('uncaughtException')event. This has the advantage of catching situations that may escape the current call stack, so is a bit more reliable than wrapping callbacks in a try/catch. However, it still depends on usingMakeCallbackas the singe entry point from C++ to JavaScript, where we use a TryCatch in C++ to route errors to the FatalException exit point.Performance
There is a slight performance increase owing to the MakeCallback refactoring, but it's not very significant. The cleanup I did was rather naive, and further performance benefits can perhaps be attained by a more thorough cleanup.
Running a http_simple benchmark against /bytes/1024 shows that they're in roughly the same ballpark. These are pretty typical results, and no significant degradation can be detected.
Note that the http_simple.js benchmark in this branch actually creates a domain and adds both the request and response objects to it explicitly, so it is a meaningful real-world use of domains. Of course, if errors are encountered, then this causes a bit of a performance hit, but otherwise, any impact is less than ambient noise.
On master (that is, 93cefab)
After the MakeCallback refactoring, but before the actual domain work:
This branch:
Shortcomings
domain.dispose()mechanism is just kludgey. We really need a single way to forcibly shut down handles, requests, and streams, so that we don't leave timers and fd's dangling. Also on the subject of debuggability, it would be nice to ensure that we don'tclose()a file descriptor while there are still outstanding fs operations on it. This consistency/cleanup can wait, however, and we can make v0.9 all about API polishing.domain.dispose(), it disposes the domain itself (ie, renders it a no-op) but it can only.destroy()andclearTimeoutthe items that have been explicitly added by callingdomain.add(emitter), not those that are implicitly bound. It would be nice to find some clever way to have the domain cheaply know which items were created during its reign, but I didn't find one.