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

Callbacks get called twice if they throw an error #76

Closed
rgjamkhedkar opened this issue Apr 26, 2013 · 12 comments
Closed

Callbacks get called twice if they throw an error #76

rgjamkhedkar opened this issue Apr 26, 2013 · 12 comments
Labels

Comments

@rgjamkhedkar
Copy link

[Edited title. Original: "getNodeById callback called multiple times". —@aseemk]

I have this code:

DataObject.get = function(id, callback) {
db.getNodeById(id, function(err, node) {
if (err) {
return callback(err);
}
return callback(null, node);
});
};

In this, callback function to getNodeById is called multiple times, I don't know why. I know for sure that DataObject.get function is called only once.
First time callback is called, I get result as expected. But when it is called second time I get an error : ReferenceError: param is not defined.

Is this a problem in node-neo4j code or in my code? I am certainly not an expert in node.js, so I might well be making a mistake.

@aseemk
Copy link
Member

aseemk commented Apr 26, 2013

Did you figure out the issue?

@blevine
Copy link

blevine commented Apr 26, 2013

I'll bet this is caused by the issue we discussed some time ago where streamline is catching all exceptions in order to return them in the callback as errors. If an exception occurs in the callback passed to DataObject.get(), that callback will be called a second time.

@aseemk
Copy link
Member

aseemk commented Apr 26, 2013

Good memory, @blevine. @rgjamkhedkar closed this, so I assume you found that to be the issue?

@rgjamkhedkar
Copy link
Author

Yes @blevine. That was the issue. I didn't know this could happen. Sorry for that, I should have checked that first.

@aseemk
Copy link
Member

aseemk commented Aug 6, 2013

I'm going to reopen this and see if I can prevent the callback from getting called twice in the case that the callback throws an error.

@aseemk aseemk reopened this Aug 6, 2013
aseemk added a commit that referenced this issue Oct 2, 2013
If the callback throws a synchronous error the first time. Issue #76.
@aseemk
Copy link
Member

aseemk commented Oct 2, 2013

Okay, spent some time investigating this, and I think it's indeed a Streamline bug:

Sage/streamlinejs#168

If that's right and it's fixed there, then this should naturally get fixed in the next recompile/release.

I've added a test case to easily test this in the future. Thanks again guys.

@tc
Copy link

tc commented Oct 17, 2013

I'm seeing this too, possible to see a new release for this fix?

Thanks

@aseemk
Copy link
Member

aseemk commented Oct 18, 2013

I'm def interested in getting the fix out for this. Just working with the Streamline.js author on it, and waiting for the fix in Streamline to be published to npm. Hopefully soon! Cheers.

@devniel
Copy link

devniel commented Nov 8, 2013

I have this issue too, it was very difficult to realize that node-neo4j is causing it. I thought that it was async.js (http://stackoverflow.com/questions/19844550/async-js-error-flow-explanation) but with this little test all my doubts were resolved : http://pastebin.com/aNcNWjLp .

Thanks

UPDATE : Thanks for link to the streamlinejs issue, I installed the last version 0.10.0 and it's fixed. I think that is important to update the npm package of node-neo4j too . Thanks 👍

aseemk added a commit that referenced this issue Nov 11, 2013
aseemk added a commit that referenced this issue Nov 11, 2013
Should address issue #76, but that test is still failing. Investigating...
aseemk added a commit that referenced this issue Nov 11, 2013
@aseemk
Copy link
Member

aseemk commented Nov 13, 2013

Thanks for your patience guys. Streamline 0.10 doesn't quite fix this 100%, but I've been working with Bruno on it.

@aseemk
Copy link
Member

aseemk commented Nov 17, 2013

Update and good news on this: Bruno found the exact cause of this, and he has fixed it. As soon as the fix gets published to npm, I'll release an update as well!

aseemk added a commit that referenced this issue Nov 17, 2013
Short-circuit test for now since it still appears to fail to Mocha, but
we can see manually/visually that it doesn't. Need to figure out a way.
@aseemk aseemk closed this as completed Nov 17, 2013
@aseemk
Copy link
Member

aseemk commented Nov 17, 2013

Okay, this has been fully fixed and published to npm as version 1.1.0!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants