Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

domains mishandle thrown nulls #6757

Closed
wants to merge 3 commits into from

3 participants

@rlidwka

This code throws user out of repl in 0.11.x:

bash$ node
> process.nextTick(function(){throw null})
undefined
> 
repl:1
(process.nextTick(function(){throw null})
                             ^
null
bash$

... as opposed to "throw 42" that doesn't.

Yeah, I know, throwing non-errors isn't nice, and shame on anybody who does that in real code.

Note unrelated to this project: this pull request was created without any issues and/or previous discussions created anywhere. I don't expect to find anybody who gives a damn about that (ref).

test/simple/test-domain.js
@@ -261,3 +261,20 @@ assert.equal(result, 'return value');
var fst = fs.createReadStream('stream for nonexistent file')
d.add(fst)
expectCaught++;
+
+
+;[42, /*NaN,*/ null, undefined, function(){}, 'string'].forEach(function(something) {
@indutny Owner
indutny added a note

Why NaN is commented out?

@rlidwka
rlidwka added a note

NaN fails because assert.strictEqual(NaN, NaN) throws. I completely forgot about that fact after discussing Object.is() in es-discuss so had some nice time debugging it. :)

It's not a bug in node, and you can safely remove that if you want. Should I add a trivial commit removing that? Or check for NaN explicitly although it isn't worth it.

@indutny Owner
indutny added a note

Yeah, would be good.

@rlidwka
rlidwka added a note

@indutny , done. I just have no idea how are you squashing commits, and do I cause less or more trouble adding one. :)

@trevnorris Owner

drop the ; at the beginning of the line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@indutny
Owner

Generally, LGTM. But please fix one nit.

@trevnorris
Owner

First make sure it's impossible to land in an infinite throwing loop if you throw null from a domain's error handler.

@rlidwka

@trevnorris , are you talking about a possibility of similar errors happening elsewhere? Or about an assumption in the core that er.domain property will exist for all errors?

I'm already seeing an issue in repl (add patch here or is it another PR?), but nothing infinitely throwing so far.

@trevnorris trevnorris was assigned
@trevnorris
Owner

@rlidwka I was thinking something like this, but until now wasn't able to check:

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

d.on('error', function() {
  throw null;
});
e.on('error', function() {
  throw null;
});

d.run(function() {
  e.run(function() {
    throw null;
  });
});

But looks like this isn't a problem.

lib/domain.js
@@ -56,8 +56,10 @@ var listenerObj = {
if (domain._disposed)
return true;
- er.domain = domain;
- er.domainThrown = true;
+ if (util.isObject(er) || util.isFunction(er)) {
@trevnorris Owner

I'd switch this to if (!util.isPrimitive(er)) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@trevnorris
Owner

@rlidwka Two things. If you want to fix them, cool. Or I can fix them in the merge. Let me know what you'd like to do and I'll get this in.

@rlidwka

If you want to fix them, cool.

@trevnorris, done. Omitting a leading semicolon in ambiguous cases is prone to errors, but I suppose linting tool will spot that anyway.

I noticed a similar 'null-as-an-error' issue in a different subsystem (repl), is it a matter of another pull request?

@trevnorris
Owner

@rlidwka If there's an issue with throwing null in repl then i'd make that another PR. it'll simplify getting this in sooner.

@rlidwka

okay, looks ready to merge then

@trevnorris
Owner

Thanks. Merged in 42a33c1.

@trevnorris trevnorris closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 24, 2013
  1. @rlidwka
Commits on Dec 25, 2013
  1. @rlidwka

    removed NaN from tests

    rlidwka authored
Commits on Jan 4, 2014
  1. @rlidwka

    fixing two things

    rlidwka authored
This page is out of date. Refresh to see the latest.
Showing with 21 additions and 2 deletions.
  1. +4 −2 lib/domain.js
  2. +17 −0 test/simple/test-domain.js
View
6 lib/domain.js
@@ -56,8 +56,10 @@ var listenerObj = {
if (domain._disposed)
return true;
- er.domain = domain;
- er.domainThrown = true;
+ if (!util.isPrimitive(er)) {
+ er.domain = domain;
+ er.domainThrown = true;
+ }
// wrap this in a try/catch so we don't get infinite throwing
try {
// One of three things will happen here.
View
17 test/simple/test-domain.js
@@ -261,3 +261,20 @@ assert.equal(result, 'return value');
var fst = fs.createReadStream('stream for nonexistent file')
d.add(fst)
expectCaught++;
+
+
+[42, null, undefined, false, function(){}, 'string'].forEach(function(something) {
+ var d = new domain.Domain();
+ d.run(function() {
+ process.nextTick(function() {
+ throw something;
+ });
+ expectCaught++;
+ });
+
+ d.on('error', function(er) {
+ assert.strictEqual(something, er);
+ caught++;
+ });
+});
+
Something went wrong with that request. Please try again.