Patch process.nextTick()

[sjelin]

node's process.nextTick() is used by a lot of libraries (q, for instance) but isn't patched by zone.js:

require('zone.js');
Zone.current.fork({}).run(() => {
  Zone.current.foo = 'bar';
  process.nextTick(() => {
    console.log(Zone.current.foo); // undefined
  });
})

process.nextTick was basically setImmediate before setImmediate existed. Now it's like setImmediate except it gets scheduled in a higher priority queue.

[JiaLiPassion]

Yeah, I agree with you, zone.js should patch process.nextTick, process.nextTick is a microtask , if we patch it like
var nativeNextTick = process.nextTick; process.nextTick = function() { var args = arguments; args[0] = Zone.current.wrap(args[0], 'process.nextTick'); nativeNextTick.apply(this, args); }

the callback will be inzone.
and the nextTick will run before setImmediate/setTimeout but after promise.resolve, because ZoneAwarePromise will not use v8 native microtask queue, should zone.js also push process.nextTick into microtask queue of zone?

such as the following case.

`
Promise.resolve().then(function() {console.log('promise resolve')});

process.nextTick(function(){console.log('process nexttick')});
`

should printout

process nexttick
promise resolve

but with zone.js patched, the result is reversed.

[sjelin]

I don't think wrapping will effect the order at all? Zone.wrap doesn't make a function asynchronous, and the callback is being scheduled by process.nextTick in either case. But I think your patch is fine regardless. (I know very little about how zones handle microtasks)

[JiaLiPassion]

yes, the zone.wrap will not change the order, what I want to say is patch it just like the code I posted can only resolve the issue that process.nextTick callback not inzone, but it will still be executed after promise.resolve(). such behaviors are different with the native implementations.

[sjelin]

After reading more about micro tasks, I agree.

[sjelin]

I think we need another task queue, sadly

[JiaLiPassion]

I believe in nodejs native implementation process.nextTick and promise.resolve use the same microtask queue, so maybe we can patch process.nextTick using the ZoneAwarenessPromise microTaskQueue...

[sjelin]

Nope:

var p = (s) => { Promise.resolve().then(() => { console.log(s); }); }
var n = (s) => { process.nextTick(() => { console.log(s); }); }

n(1);
p(3);
n(2);
p(4);

Outputs:

1
2
3
4

On node v6.9.1

[sjelin]

So I guess your warping is fine?

[JiaLiPassion]

in native node without zone.js the result is the same as yours, but with zone.js
I test in mac, node 7.0, the result is

3 4 1 2

[sjelin]

I switched to v7.1.0 but still got the same order (I'm on Ubuntu and I left my mac at home).

Regardless, it's more complicated: nodejs/node-v0.x-archive#8325

Basically, it empties one queue, then switches to the other. It keeps switching back and forth until both queues are empty. For instance, on my machine the following:

var p = (c) => { return Promise.resolve().then(c); }
var n = (c) => { process.nextTick(c); }
var q = (s) => {return p(() => {console.log(s + ' (promise)');});}
var m = (s) => {return n(() => {console.log(s + ' (next tick)');});}

m(1);
q(3);
n(() => {
  m(2);
  q(4);
});
p(() => {
  m(6);
  q(5);
});

Outputs:

1 (next tick)
2 (next tick)
3 (promise)
4 (promise)
5 (promise)
6 (next tick)

For some reason on my machine it starts with the nextTick queue and on yours it starts with the MicroTask queue. It sounds from the PR like the way my machine works is normal, though everything else I've read would indicate that your machine is the normal one. On your machine I suspect you'll get:

3 (promise)
5 (promise)
1 (next tick)
6 (next tick)
2 (next tick)
4 (promise)

Regardless, we definitely need a second queue

[JiaLiPassion]

yeah, I saw this issue too, nodejs/node-v0.x-archive#8325, from this PR, in my understanding they use node v8 microtask to proceed the process.nextTick.

I will test your code, and see what is the order.

[JiaLiPassion]

Yes, I my machine with zone.js the result is
3 (promise)
5 (promise)
1 (next tick)
6 (next tick)
2 (next tick)
4 (promise)

[sjelin]

Oh sorry, I was reading quickly and didn't see that you only got the weird order with zone.js

[sjelin]

I actually loaded up zone.js (at 3b94918) on my ubuntu machine and I still get the 1 2 3 4 5 6 order. Maybe it's just your PR that changes the order then?

[JiaLiPassion]

yes, it seems the patch change the order... I'll check it to find why..

[JiaLiPassion]

I write code like following , the order is still correct.
So I think maybe the Zone.js patch jasmine cause the incorrect order. (the wrong order only occurs when I put the case into jasmine cases of the zone.js repository)

require('./zone-node.js');
var nativeNextTick = process.nextTick;
process.nextTick = function() {
  var args = arguments;
  args[0] = Zone.current.wrap(args[0], 'process.nextTick');
  nativeNextTick.apply(this, args);
}

var p = (c) => { return Promise.resolve().then(c); }
var n = (c) => { process.nextTick(c); }
var q = (s) => {return p(() => {
    console.log(s + Zone.current.name + ' (promise)');
});}

var m = (s) => {return n(() => {console.log(s + Zone.current.name + ' (next tick)');});}

Zone.current.fork({name: 'A'}).run(function() {
m(1);
q(3);
n(() => {
  m(2);
  q(4);
});

[sjelin]

Oh yeah, I wasn't running inside jasmine. That could be it.

[sjelin]

I did some experimentation:

• I also get the wrong order when I put my tests within zone.js's jasmine tests
• The order is fixed when I comment out https://github.com/angular/zone.js/blob/master/test/test-env-setup-jasmine.ts#L10
• The order is broken again if I require('../../lib/jasmine/jasmine') in the spec file, directly before the it() block which uses process.nextTick
• However, if I move the require inside the it() block it doesn't cause any problems

[sjelin]

Found the problem: https://github.com/angular/zone.js/blob/master/lib/jasmine/jasmine.ts#L132

As you can see, the jasmine patch runs the tests within a microtask, which therefore makes the microtask queue the active queue, which is why microtasks are being run before nextTick callbacks. I don't actually think this is a problem.

[JiaLiPassion]

Yeah! That is the problem, so the order should be correct in real code.

[sjelin]

Yeah, I say we go with your original suggestion for how to patch

[JiaLiPassion]

Yeah, I have make a PR #516, and add several cases.

[JiaLiPassion]

I change the PR to use zone.scheduleMicroTask to patch the process.nextTick,

function patchNextTick() {
  let setNative = null;

  function scheduleTask(task: Task) {
    const args = task.data;
    args[0] = function() {
      task.invoke.apply(this, arguments);
    };
    setNative.apply(process, args);
    return task;
  }

  setNative =
      patchMethod(process, 'nextTick', (delegate: Function) => function(self: any, args: any[]) {
        if (typeof args[0] === 'function') {
          const zone = Zone.current;
          const task = zone.scheduleMicroTask('nextTick', args[0], args, scheduleTask);
          return task;
        } else {
          // cause an error by calling it directly.
          return delegate.apply(process, args);
        }
      });
}

so we can use zoneSpec.onScheduleTask/onInvokeTask to trace the Task lifecycle.
https://github.com/angular/zone.js/pull/516/files#diff-5451e0d148fd2cc1adaf94b9ac70c0a8R37