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

--abort-on-uncaught-exception prevents domains from working at all (the process crashes) #836

Closed
geek opened this issue Feb 13, 2015 · 8 comments
Labels
confirmed-bug Issues with confirmed bugs.

Comments

@geek
Copy link
Member

geek commented Feb 13, 2015

It was fixed in node: nodejs/node-v0.x-archive#8631

This is a major issue that is preventing anyone from using domains and the very powerful --abort-on-uncaught-exception flag.

@Fishrock123
Copy link
Contributor

cc @cjihrig?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 13, 2015

The relevant PR is nodejs/node-v0.x-archive#8666, where @trevnorris expressed that the solution wouldn't completely work for node 0.12 because of promises. The solution also involved floating the v8 patch nodejs/node-v0.x-archive@fbff705, which we obviously don't want to do.

I don't know the best way to proceed here. I'd like to hear from others in @iojs/tc

EDIT: I think I saw something in IRC a while back about explicitly not porting this over.

@bnoordhuis
Copy link
Member

I commented on that here. To summarize, it seems to me that --abort_on_uncaught_exception is working as intended.

Let's take a step back and outline what the desired behavior is. Dump core on uncaught exceptions except when there is an active domain?

@cjihrig
Copy link
Contributor

cjihrig commented Feb 14, 2015

@geek may want to chime in here, but I would expect the following program to behave the same with, and without, the --abort_on_uncaught_exception flag. With the flag, the process aborts. Without it, the domain is able to catch the error.

var domain = require('domain');
var d = domain.create();

d.on('error', function(err) {
  console.log('domain caught ' + err);
});

d.run(function() {
  throw new Error('foo');
});

@geek
Copy link
Member Author

geek commented Feb 14, 2015

My expectation is that a core is created whenever a process would normally crash. The flag should not cause a domain to stop working.

Without this fixed, how are you expected to do any post mortem debugging and use domains in your application?

@bnoordhuis
Copy link
Member

Right, I think this is a flaw in the domains implementation, possibly coupled with a misunderstanding of what --abort_on_uncaught_exception does. That flag means 'abort when there is no JS try/catch block on the stack' and indeed there isn't one in domain.run(). The patch below makes @cjihrig's test case work but it's not a general solution.

The test case fails again when you wrap the throw in a process.nextTick() and that's because _tickDomainCallback() in src/node.js doesn't have a try/catch block, it only has a try/finally block. If you catch the exception, the process no longer aborts. Ditto for every other place where callbacks are invoked.

Whoever wants to work on fixing this should probably explore alternatives and write up a change proposal first because just blindly adding try/catch blocks everywhere is not a great idea.

diff --git a/lib/domain.js b/lib/domain.js
index c666fb5..5f8e6d4 100644
--- a/lib/domain.js
+++ b/lib/domain.js
@@ -183,16 +183,20 @@ Domain.prototype.run = function(fn) {
   var ret;

   this.enter();
-  if (arguments.length >= 2) {
-    var len = arguments.length;
-    var args = new Array(len - 1);
+  try {
+    if (arguments.length >= 2) {
+      var len = arguments.length;
+      var args = new Array(len - 1);

-    for (var i = 1; i < len; i++)
-      args[i - 1] = arguments[i];
+      for (var i = 1; i < len; i++)
+        args[i - 1] = arguments[i];

-    ret = fn.apply(this, args);
-  } else {
-    ret = fn.call(this);
+      ret = fn.apply(this, args);
+    } else {
+      ret = fn.call(this);
+    }
+  } catch (er) {
+    this.emit('error', er);
   }
   this.exit();

@geek geek added the confirmed-bug Issues with confirmed bugs. label Feb 16, 2015
@geek
Copy link
Member Author

geek commented Feb 16, 2015

@bnoordhuis hopefully this bug will get fixed soonish... it's definitely keeping me from wanting to switch to io.js for production.

What do people use for post mortem debugging in production if they aren't using core files? I've tried heap snapshots, but the /proc tooling in SmartOS is incredibly useful.

chrisdickinson added a commit that referenced this issue Feb 25, 2015
If run with --abort-on-uncaught-exception, V8 will abort the process
whenever it does not see a JS-installed CatchClause in the stack. C++
TryCatch clauses are ignored. Domains work by setting a FatalException
handler which is ignored when running in abort mode.

This patch modifies MakeCallback to call its target function through a
JS function that installs a CatchClause and manually calls _fatalException
on error, if the process is both using domains and is in abort mode.

Semver: patch
PR-URL: #922
Fixes: #836
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@dougwilson
Copy link
Member

What do people use for post mortem debugging in production if they aren't using core files?

Simply write programs without flaws :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs.
Projects
None yet
Development

No branches or pull requests

5 participants