Functions of Database and Statement should be domain-aware

[FrozenCow]

This is based on a earlier pull request that became stale a year ago: #81

It is about NodeJS domains: http://nodejs.org/api/domain.html

At the moment callbacks from Database and Statement are not domain-aware. That means that when a call was made from within a domain, the callback of that call won't be in that same domain. Exceptions that are thrown inside such a callback will be caught by process.on('uncaughtException') instead of domain.on('error'). This can be confusing for developers that make use of domains for exceptions or logging.

I used the commits from @chrisdickinson as a base and converted the test to Mocha.
I've also incorporated the comments from the mentioned pull request, which basically said to make use of EventEmitter to set domain, instead of manually setting the domain. To do this I had to call back to JS from the native Database/Statement C++ constructors.
Database#exec is not implemented using Statement, so the callback for that function needed to be wrapped in order to pass the domain.

Side note:
This is a step in the right direction I think, but this doesn't make everything of node-sqlite3 domain-aware. The calls in Statement are purely native and I wasn't sure how to store and bind the domain to the callback. This is however quite easy if these functions were wrapped in a Javascript. Database#exec is an example for that.
That said, problems/confusion will only arise once a statement is created in one domain (A), but the statement is used in another (B). You would think callbacks will be in domain B, but they will be in domain A with these commits.

[FrozenCow]

The profiling test apparently failed when using MakeCallback instead of Call. I'm not totally sure why, but apparently assert.ok(create); in the test was called before the profile event triggered. I figured the profiling test should allow more than just one event-loop iteration.

[FrozenCow]

Hmm, the domain-tests are failing on Travis since it is using the prebuilt version from s3. The tests do work fine with npm install --build-from-source.

[FrozenCow]

@springmeyer Can I get some feedback on the code? What can I do to get this in the next release of node-sqlite3?

[springmeyer]

I've personally not used domains so I'm not in a good position to review. But I'm inclined to accept the patch if others support it.

On Feb 24, 2014, at 5:48 PM, FrozenCow notifications@github.com wrote:

@springmeyer Can I get some feedback on the code? What can I do to get this in the next release of node-sqlite3?

—
Reply to this email directly or view it on GitHub.

[springmeyer]

this comment (nodejs/node-v0.x-archive#5798 (comment)) seems to indicate potentially more overhead to domains + nextTick usage. Would be good to benchmark to ensure this is note noticeable.

[FrozenCow]

What this patch does is mostly changing calls to make use of MakeCallback. Apart from that it calls the EventEmitter constructor on Database and Statement instances, which could be seen as a separate fix, but doesn't impact performance in the way the linked comment describes.

That said, I can imagine MakeCallback being less performant than the current way of calling functions. However it seems another open pull request (#184) also suggests to use MakeCallback. If MakeCallback is used, we're already in a way better position in terms of supporting domains. This will fix the issue of node-sqlite3 callbacks from not propagating domains. As for performance of domains specifically: MakeCallback checks whether domains are being used and if so, it'll do a MakeDomainCallback (https://github.com/joyent/node/blob/master/src/node.cc#L1055). Applications that don't use domains shouldn't have any overhead, applications that do use domains have to face the consequences anyway ;).

[hillmanov]

Looks like this hasn't been discussed in a while - is this still something that is going to be considered? If not, is there a recommended approach to handling errors that are thrown that are non-catchable with try/catch or with domains?

[springmeyer]

Looks like this hasn't been discussed in a while - is this still something that is going to be considered?

@hillmanov I don't use domains / have much experience with them, so I've been hesitant to accept this but I'm open to it still.

If not, is there a recommended approach to handling errors that are thrown that are non-catchable with try/catch or with domains?

Can you explain what kind of errors these are?

[FrozenCow]

Domains shouldn't be used to actually catch errors, just to log/know in what context the errors happened (after handling the error you should kill the process anyway). It is also now deprecated in the latest Node versions: https://nodejs.org/api/domain.html See the discussion here: nodejs/node#66

An alternative would be https://github.com/btford/zone.js/, but I haven't given it a try yet.

[wmertens]

in Node v9, domains are deprecated…