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

the async API breaks process.nextTick() within the callback #14

Closed
bman654 opened this issue Jul 2, 2015 · 11 comments
Closed

the async API breaks process.nextTick() within the callback #14

bman654 opened this issue Jul 2, 2015 · 11 comments
Labels

Comments

@bman654
Copy link

bman654 commented Jul 2, 2015

I discovered this issue when I wrapped eval(expr,cb) into a Promise-returning API. Promises execute their callbacks via process.nextTick()

If you call process.nextTick(foo) from within the evaluation callback, then foo will not execute until something else happens (such as another timeout expires).

See these Mocha tests:

import * as julia from 'node-julia';
import { expect } from 'chai';

describe('node-julia', () => {
    it('should work asynchronously', done => {
        julia.eval("2+3", (err, result) => {
            if (!err) {
                expect(result).to.equal(5);
            }
            done(err);
        });
    });

    it('should not mess up process.nextTick() when called asynchronously', done => {
        julia.eval("2+3", (err, result) => {
            if (!err) {
                expect(result).to.equal(5);
            }

            process.nextTick(() => done(err));
        });
    });

    it('should not require calling setTimeout() to fix process.nextTick() when called asynchronously', done => {
        julia.eval("2+3", (err, result) => {
            if (!err) {
                expect(result).to.equal(5);
            }

            process.nextTick(() => done(err));
            setTimeout(() => {}, 0);
        });
    });
});

The first test passes.
The second test fails (times out)
The third test (where I added an empty setTimeout) passes.

It seems like it is related to this issue: nodejs/node-v0.x-archive#7714

I'm guessing your callbacks are somehow bypassing the nodejs event loop and so the event loop does not know to drain the nextTick queue until something else happens to wake up the event loop.

@bman654
Copy link
Author

bman654 commented Jul 2, 2015

FYI this is the JavaScript stack trace I get if I print it via (console.log(new Error().stack)).

Inside my callback:

at /.../node-julia-test.es6:17:36

Inside my nextTick callback:

at /.../node-julia-test.es6:28:61
at process._tickCallback (node.js:355:11)

@bman654
Copy link
Author

bman654 commented Jul 2, 2015

node-julia version: node-julia@1.1.2 (git://github.com/wateim/node-julia.git#73eb78f3873eeaafa96e4c81b7b5560f30e35435)
node version: v0.12.5
OS: Linux ubuntu-14 3.13.0-24-generic #46-Ubuntu SMP Thu Apr 10 19:11:08 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux

@sebastiang
Copy link
Contributor

and if it helps, JuliaLang/julia@fe7203e

@waTeim
Copy link
Owner

waTeim commented Jul 2, 2015

I don't remember doing anything non standard as far as async notification. Just basic uv_async_send, but I do remember there can be only 1 notify delivery for multiple things in uv_async_send so knowing that there's a similar thing that happens in nj maybe something subtle about that. This might prove difficult as inserting logging will alter the eventing behavior. Might be worth attempting with iojs just for perspective and (lack of) reproducibility. BTW just for info, are you using Q or bluebird or something else for the promises wrapping?

@sebastiang
Copy link
Contributor

I'm guessing that it may not be enough to notify libuv about your callback, but you may need to specifically kick in nodejs's bookkeeping, e.g. whatever's going on with process._tickCallback

@bman654
Copy link
Author

bman654 commented Jul 2, 2015

It looks like one way or another you need to call MakeCallback() to actually run the callback and allow nodejs to trigger its nextTick processing logic (and also its domain logic which I also notice is not working with node-julia.

It looks like all of the node native libraries use this method. Here's the timers, for example: https://github.com/joyent/node/blob/master/src/timer_wrap.cc#L140

FYI, here's some unit tests showing the callbacks also are not running on the current domain:

    it('this one works', done => {
        const d = domain.create();
        d.on('error', () => done());
        d.run(() => {
            process.nextTick(() => { throw new Error("some error"); });
        });
    });
    it('this one fails', done => {
        const d = domain.create();
        d.on('error', () => done());
        d.run(() => {
            julia.eval("2+3", () => { throw new Error("some error"); });
        });
    });

@bman654
Copy link
Author

bman654 commented Jul 2, 2015

Re: promises. We use RxJs for most of our async stuff so I chose not to pull in a library like bluebird or Q.

So I just use this function to wrap:

/**
 * Converts a Node.js callback style function to a Promise.  This must be in function (err, ...) format.
 * @param {Function} func The function to call
 * @param {Function} [selector] A selector which takes the arguments from the callback minus the error to produce a single item to yield on next.
 * @returns {Function} An async function which when applied, returns a Promise with the callback results (if there are multiple results, then they are supplied as an array)
 */
export function fromNodeCallback(func, selector) {
    var newFunc = function (...args) {
        return new Promise((resolve, reject) => {
            func(...args, (err, ...results) => {
                if (err) {
                    reject(err);
                }
                else {
                    let finalResults = results.length > 1 ? results : results[0];
                    // run the selector if provided
                    if (selector) {
                        try {
                            finalResults = selector(finalResults);
                        }
                        catch (e) {
                            reject(e);
                        }
                    }
                    resolve(finalResults);
                }
            });
        });
    };

    newFunc.displayName = func.displayName;
    return newFunc;
};

Usage:

const evalAsync = fromNodeCallback(julia.eval);
const myPromise = evalAsync("2+3");

@waTeim
Copy link
Owner

waTeim commented Jul 2, 2015

Hey sorry for the catch up questions. Are you guys using a transpiler like traceur or babel, I was able to get mocha to process the test but only after changing back to the using require rather than import.

@bman654
Copy link
Author

bman654 commented Jul 2, 2015

yes I'm using babel + babel runtime + webpack.

@waTeim
Copy link
Owner

waTeim commented Jul 3, 2015

I believe a7cbe3b has addressed this problem, the test case passes, and according to nodejs/nan#284 and this stackoverflow question, it's the right approach.

@waTeim waTeim added the bug label Jul 3, 2015
@bman654
Copy link
Author

bman654 commented Jul 6, 2015

Yes this checkin resolves the nextTick issue. The domain test still fails. I'll open a different issue for that.

Thanks

@bman654 bman654 closed this as completed Jul 6, 2015
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

3 participants