async_hooks: use parent promise as triggerId in init hook #13367

JiaLiPassion · 2017-06-01T11:33:57Z

async_hooks init callback will be triggered when promise newly created,
in previous version, the parent promise which pass from chrome V8 PromiseHook
is ignored, so we can't tell the promise is a pure new promise or a
chained promise.

In this commit, we use the parent promise's id as triggerId to
trigger the init callback.

Fixes: #13302

Thanks for @AndreasMadsen and @addaleax 's great help!

Please review.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

AndreasMadsen

I think this is a good idea, but you asked about how to detect if a promise was created without a parent promise.

Can I say that triggerId is 1 means this Promise is just a newly created Promise without any parent chain?

Not always, triggerId = 1 is just the trigger_id for the root, if you create a promise inside the callback of another asynchronous operation you will get another trigger_id.

If we implement as suggested, you can check if triggerId === async_hooks.currentId(). If that is true then the promise has no parent (there might be an edge case I haven't thought of).

I have thought some more about it, and the triggerId === async_hooks.currentId() condition is not sufficient it is just necessary (sorry for the math language).

Consider the following code:

const p0 = new Promise((resolve) => resolve(1));
const p1 = p0.then(function () {
  // async_hooks.currentId() === asyncId(p1)
  p2 = p1.then(function () { // trigger_id = asyncId(p1)
    
  });
  return 2;
});

In this case async_hooks.currentId() == trigger_id but p2 is a parent of p1.

I still think setting the trigger_id like this is useful, but it doesn't allow for detecting if there is a parent promise.

A possible solution could be have the resource be an actual PromiseWrap object and then store the meta information on that object. We have talked about this before.

AndreasMadsen · 2017-06-01T11:50:31Z

src/async-wrap.cc

+      // current promise's triggerId
+      double trigger_id = Unwrap<PromiseWrap>(parent.As<Promise>())->get_id();
+      env->set_init_trigger_id(trigger_id);
+      wrap = new PromiseWrap(env, promise, silent);


Maybe just

+ // set parent promise's async Id as this promise's triggerId + if (parent->IsPromise()) { + // parent promise exists, current promise + // is a chained promise, so we set parent promise's id as + // current promise's triggerId + double trigger_id = Unwrap<PromiseWrap>(parent.As<Promise>())->get_id(); + env->set_init_trigger_id(trigger_id); + } wrap = new PromiseWrap(env, promise, silent);

@AndreasMadsen , Thank very much for the review, I have modified this one.

JiaLiPassion · 2017-06-01T13:35:27Z

@AndreasMadsen , about the sample code you provided,

Consider the following code:

const p0 = new Promise((resolve) => resolve(1));
const p1 = p0.then(function () {
  // async_hooks.currentId() === asyncId(p1)
  p2 = p1.then(function () { // trigger_id = asyncId(p1)
    
  });
  return 2;
});

I make a test with this PR compiled node.

const async_hooks = require('async_hooks');

function init(id, provider, parentId, parentHandle) {
    process._rawDebug('init: ', id, 'triggerId: ', parentId, 'currentId',  async_hooks.currentId());
}

function before(id) { 
    process._rawDebug('before', id, async_hooks.currentId());
 }
function after(id) { 
    process._rawDebug('after', id, async_hooks.currentId());
 }
function destroy(id) {
    process._rawDebug('destroy', id);
}

async_hooks.createHook({init, before, after, destroy}).enable();
debugger;
const p = new Promise ( resolve => {
    process._rawDebug('execute p');
    resolve(1);} );
const p1 = p.then(val => { 
    process._rawDebug('p1 then', val); 
    const p2 = p1.then((val1) => {
      process._rawDebug('p2 then', val1); 
    });
    return 2; });

the output is

init:  2 triggerId:  1 currentId 1
execute p
init:  3 triggerId:  2 currentId 1
before 3 1
p1 then 1
init:  4 triggerId:  3 currentId 1
after 3 1
before 4 1
p2 then 2
after 4 1

p is promise without parent, so triggerId === currentId
p1 is promise , and p1's parent is p, so the triggerId is 2
p2 is promise , and p2's parent is p1, so the triggerId is 3,

I think this a correct result.

And I tried another case

setTimeout(() => {
    const p = new Promise(resolve => {
        process._rawDebug('execute p');
        resolve(1);
    });
    const p1 = p.then(val => {
        process._rawDebug('p1 then', val);
        const p2 = p1.then((val1) => {
            process._rawDebug('p2 then', val1);
        });
        return 2;
    });

}, 100);

the output is

init:  2 type: Timeout triggerId:  1 currentId 1
init:  3 type: TIMERWRAP triggerId:  1 currentId 1
before 3 3
before 2 2
init:  4 type: PROMISE triggerId:  2 currentId 2
execute p
init:  5 type: PROMISE triggerId:  4 currentId 2
after 2 2
after 3 3
before 5 0
p1 then 1
init:  6 type: PROMISE triggerId:  5 currentId 0
after 5 0
before 6 0
p2 then 2
after 6 0
destroy 2

I think the result is also ok, but I don't quite understand the why currentId will become 0.

Please review. Thank you very much!

AndreasMadsen · 2017-06-01T14:13:29Z

Interesting, I think something is wrong with async_hooks.currentId().

init:  2 triggerId:  1 currentId 1
execute p
init:  3 triggerId:  2 currentId 1
before 3 1
p1 then 1
init:  4 triggerId:  3 currentId 1 // wrong, currentId should be 3
after 3 1
before 4 1
p2 then 2
after 4 1

PS: I very much doubt this change is responsible for the bug.

/cc @nodejs/async_hooks

JiaLiPassion · 2017-06-01T14:22:43Z

@AndreasMadsen, I am not sure I understand the lifecycle of currentId, so I do not know what is the correct result,just like the another sample involve setTimeout will have a currentId 0,I do not understand why.

AndreasMadsen · 2017-06-01T14:28:13Z

@JiaLiPassion The async_hooks.currentId() should just be the id from the latest before emit that hasn't been closed by the after emit.

In some cases, there is no before emit (and it is also not the root) then currentId will be 0, but that is not the case in both examples.

JiaLiPassion · 2017-06-01T14:36:26Z

@AndreasMadsen , Thank you for the explanation.

The async_hooks.currentId() should just be the id from the latest before emit that hasn't been closed by the after emit.

Yeah, this is making sense.

In some cases, there is no before emit (and it is also not the root) then currentId will be 0, but that is not the case in both examples.

you mean there is only init emit, is that right?

And yes, in my setTimeout example, there is before emit, so I don't know why there is a 0 currentId. ( I have tried the nightly build version of nodejs, the currentId result is the same).

AndreasMadsen · 2017-06-01T21:08:44Z

you mean there is only init emit, is that right?

For example in the TCP server you will get:

init TCP socket
before TCP server onconnection
....
after TCP server onconnection

This is because the TCP socket is created in C++ land and then send to the callback. Since this happens before any javascript the currentId is zero.

addaleax

The code change looks good to me, aside from one edge case. I still am not 100 % sure about this being the right thing, but if Andreas says so, I trust him.

I would still really like to see a test that shows that getting the original execution scope remains possible, because that is something that actual use cases will rely on.

addaleax · 2017-06-01T21:18:15Z

src/async-wrap.cc

+      // parent promise exists, current promise
+      // is a chained promise, so we set parent promise's id as
+      // current promise's triggerId
+      double trigger_id = Unwrap<PromiseWrap>(parent.As<Promise>())->get_id();


You need to also check whether Unwrap<PromiseWrap>() returned nullptr (that could happen if async_hooks were enabled after parent was created, but before promise was created).

If it is, you’ll need to basically do the same thing we’re doing just below; something along the lines of:

auto parent_wrap = new PromiseWrap(env, parent, true); parent_wrap->MakeWeak(parent_wrap);

@addaleax , thank you for the review.

So the code should look like

auto parent_wrap = Unwrap<PromiseWrap>(parent.As<Promise>()); if (parent_wrap != nullptr) { double trigger_id = parent_wrap->get_id(); env->set_init_trigger_id(trigger_id); }

@JiaLiPassion I think you would still want to create a new PromiseWrap for the parent Promise. If you do that, you don’t need to check if (parent_wrap != nullptr).

(Basically, look at the existing code here and how it adds a parent_wrap when there isn’t any. If it’s unclear, feel free to ping me and I’ll explain in more detail. :))

@addaleax , thank you so much! sorry for so many basic questions...

So the code should like

// create a new PromiseWrap for parent promise auto parent_wrap = new PromiseWrap(env, parent, true); parent_wrap->MakeWeak(parent_wrap); // get id from parentWrap double trigger_id = parent_wrap->get_id(); env->set_init_trigger_id(trigger_id);

The new PromiseWrap should only be created if Unwrap<> gives you a nullptr (otherwise there already is one, so we don’t need that). But yes, that’s the general idea.

@addaleax , thank you, so it will look like

if (parent->IsPromise()) { auto parent_wrap = Unwrap<PromiseWrap>(parent.As<Promise>()); if (parent_wrap == nullptr) { // create a new PromiseWrap for parent promise with silent parameter parent_wrap = new PromiseWrap(env, parent, true); parent_wrap->MakeWeak(parent_wrap); } // get id from parentWrap double trigger_id = parent_wrap->get_id(); env->set_init_trigger_id(trigger_id); } }

I just updated the code, ping @addaleax !

AndreasMadsen · 2017-06-01T21:29:59Z

The code change looks good to me, aside from one edge case. I still am not 100 % sure about this being the right thing, but if Andreas says so, I trust him.

Sorry to burst your bubble, but the triggerId is something I really struggle with. I tried to explain some of my confusion in #11883 (review). Unfortunately, I still haven't gotten any wiser regarding when to set triggerId and when to let it default to currentId.

Also since this doesn't solve the problem that @JiaLiPassion originally asked, regarding detecting if a parentPromise exists, I would like to explore some alternatives.

tl;dr; I think this makes sense, but I'm generally uncertain about triggerId.

I would still really like to see a test that shows that getting the original execution scope remains possible, because that is something that actual use cases will rely on.

Yeah, we need to fix the currentId bug before that can happen. I think the test case that should follow with that should be sufficient.

addaleax · 2017-06-01T21:43:10Z

@AndreasMadsen Regarding triggerId, I can definitely see that there are at least two valid perspectives:

If it’s about tracking asynchronous causality, this PR might be the right one. It would make a Promise look like it descended from where the origin of the Promise chain was created. I am not sure that’s super helpful, though; a Promise can be resolved with another Promise, breaking the causality chain, and afaik we can’t track that well currently.
If it’s about tracking asynchronous context, this PR is not the right one; basically, if you want to have the code after an await in an async fn look like it runs in a context descended from the one before await, then the current behaviour is what you want.

Writing this comment, it still seems to me like we should maybe just expose the parent Promises async id as an extra argument to the init callback.

needs more discussion :/

AndreasMadsen · 2017-06-01T21:50:23Z

@addaleax It is the first one, but there is a thin line between causality and parent resource, which is what causes my confusion.

From the documentation:

triggerId is the asyncId of the resource that caused (or "triggered") the
new resource to initialize and that caused init to call. This is different
from async_hooks.currentId() that only shows when a resource was created,
while triggerId shows why a resource was created.

JiaLiPassion · 2017-06-01T21:50:48Z

@AndreasMadsen , @addaleax ,

Writing this comment, it still seems to me like we should maybe just expose the parent Promises async id as an extra argument to the init callback.

So if expose the parent Promise async id as an extra argument, it can resolve my original issue, that if the argument exists, the the promise is a chained one, is that correct?

If it’s about tracking asynchronous context, this PR is not the right one; basically, if you want to have the code after an await in an async fn look like it runs in a context descended from the one before await, then the current behaviour is what you want.

do you mean the case like this?

async function test() {
    await new Promise(resolve => resolve(1));
}

process._rawDebug('before', async_hooks.currentId());
test();
process._rawDebug('after', async_hooks.currentId());

addaleax · 2017-06-01T21:53:04Z

@AndreasMadsen Okay, makes sense.

So if expose the parent Promise async id as an extra argument, it can resolve my original issue, that if the argument exists, the the promise is a chained one, is that correct?

@JiaLiPassion Yes, I’d say so.

do you mean the case like this?

Please keep this issue on track, this PR is just about fixing `triggerId`

@hayes will create issues for the other problems. If something is missing you are welcome to create a new issue.

JiaLiPassion · 2017-06-03T08:15:30Z

@AndreasMadsen , I have updated the test cases and assert message.
#13367 (comment)

please review.

addaleax · 2017-06-03T09:23:07Z

@JiaLiPassion Yes, that is how you’d check whether a parent promise exists. (Now that you mention it: Maybe handle.parent should refer to the parent PromiseWrap, not the parent promise itself. I’m not sure.)

JiaLiPassion · 2017-06-03T09:27:48Z

@addaleax , got it , thank you very much!!!

Would you please make another PR to implement it?

addaleax · 2017-06-03T09:33:31Z

Would you please make another PR to implement it?

@JiaLiPassion I’ll wait until you land this PR, then I can do that. :)

addaleax · 2017-06-03T10:49:48Z

test/async-hooks/test-promise.chain-promise-before-init-hooks.js

+const initHooks = require('./init-hooks');
+const { checkInvocations } = require('./hook-checks');
+
+const p = (new Promise(common.mustCall(executor)));


You don’t need the outer parentheses here

yes, I will remove it.

addaleax · 2017-06-03T10:50:30Z

test/async-hooks/test-promise.chain-promise-before-init-hooks.js

+function afterresolution(val) {
+  assert.strictEqual(val, 5);
+  return val;
+}


I think this would be a bit more readable if you just inlined executor and afterresolution

yes, you are right, I will change it.

addaleax · 2017-06-03T10:50:59Z

test/async-hooks/test-promise.chain-promise-before-init-hooks.js

+}
+
+process.on('exit', onexit);
+function onexit() {


same for onexit, using process.on('exit', () => { works just fine

yes, I will change it.

addaleax · 2017-06-03T10:52:40Z

test/async-hooks/test-promise.js

@@ -14,7 +14,7 @@ p.then(afterresolution);

 function executor(resolve, reject) {
  const as = hooks.activitiesOfTypes('PROMISE');
-  assert.strictEqual(as.length, 1, 'one activities');
+  assert.strictEqual(as.length, 1, 'one activity');


If you want to avoid merge conflicts with #13243, you can just drop this change

@addaleax , ok , so I just revert it.

addaleax · 2017-06-03T10:53:19Z

test/async-hooks/test-promise.js

@@ -46,7 +46,8 @@ function onexit() {
  const a1 = as[1];
  assert.strictEqual(a1.type, 'PROMISE', 'promise request');
  assert.strictEqual(typeof a1.uid, 'number', 'uid is a number');
-  assert.strictEqual(a1.triggerId, 1, 'parent uid 1');
+  assert.strictEqual(a1.triggerId, a0.uid,
+                     'child promise should use parent uid as triggerId');


Yeah, I would just not use an error message here (i.e. assert.strictEqual(a1.triggerId, a0.uid);). Sorry the tests are written this way.

ok, so I just modify it to

assert.strictEqual(a1.triggerId, a0.uid);

addaleax · 2017-06-03T10:56:30Z

Left a few style nits.

CI: https://ci.nodejs.org/job/node-test-commit/10333/

AndreasMadsen · 2017-06-03T11:04:20Z

I will fix these, there are some other changes I would like.

JiaLiPassion · 2017-06-03T11:06:00Z

@AndreasMadsen , ok, thank you! so I don't need to update the source which were reviewed by @addaleax ?

AndreasMadsen · 2017-06-03T11:11:45Z

@JiaLiPassion yep, I fixed it for you :)

I didn't really understand why the triggerId was what it was, so I checked out your code and investigated it. Nothing was wrong, but I made it a bit more robust for future changes.

JiaLiPassion · 2017-06-03T11:12:58Z

@AndreasMadsen , thank you very much!!! you and @addaleax give me great help!!!

AndreasMadsen

LGTM. async_hooks it a beast, so I'm happy to see that made it. I look forward to future interactions with zone.js.

CI: https://ci.nodejs.org/job/node-test-pull-request/8455/

addaleax · 2017-06-03T20:50:07Z

Landed in 8741e3c 🎉

@JiaLiPassion I believe this is your first commit to core, if so, welcome on board! Thank you for the work you have been putting into this, and I think I speak for everyone in this thread if I say that we’d love to see more of that. If you’re interested, we can probably add you to the @nodejs/async_hooks review team that gets @mentioned on issues and PRs related to async_hooks.

@jasnell I’ve left you off as a reviewer because the PR has undergone quite some change since you reviewed, and wasn’t really ready at that point anyway. I hope that’s okay.

async_hooks init callback will be triggered when promise newly created, in previous version, the parent promise which pass from chrome V8 PromiseHook is ignored, so we can't tell the promise is a pure new promise or a chained promise. In this commit, we use the parent promise's id as triggerId to trigger the init callback. Fixes: #13302 PR-URL: #13367 Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

JiaLiPassion · 2017-06-03T21:46:45Z

@addaleax , Thank you so much for all the great help, I am so glad to join here and definitely want to do more in the future.

If you’re interested, we can probably add you to the @nodejs/async_hooks review team that gets @mentioned on issues and PRs related to async_hooks.

Yes!!! I am very happy to join here and it will be my honor to join @nodejs/async_hooks review team to do more contributions and want to learn more from here.

Thank you and @AndreasMadsen again for all the great help!!!

async_hooks init callback will be triggered when promise newly created, in previous version, the parent promise which pass from chrome V8 PromiseHook is ignored, so we can't tell the promise is a pure new promise or a chained promise. In this commit, we use the parent promise's id as triggerId to trigger the init callback. Fixes: #13302 PR-URL: #13367 Reviewed-By: Andreas Madsen <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](nodejs@135f4e6643)] [nodejs#13367](nodejs#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](nodejs@968596ec77)] [nodejs#13306](nodejs#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](nodejs@ffa7debd7a)] [nodejs#13487](nodejs#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](nodejs@6e0eccd7a1)] [nodejs#13316](nodejs#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](nodejs@c756efb25a)] [nodejs#13173](nodejs#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](nodejs@cc6ec2fb27)] [nodejs#5025](nodejs#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](nodejs@6aeb555cc4)] [nodejs#13374](nodejs#13374).

* **Async Hooks** * When one `Promise` leads to the creation of a new `Promise`, the parent `Promise` will be identified as the trigger [[`135f4e6643`](135f4e6643)] [#13367](#13367). * **Dependencies** * libuv has been updated to 1.12.0 [[`968596ec77`](968596ec77)] [#13306](#13306). * npm has been updated to 5.0.3 [[`ffa7debd7a`](ffa7debd7a)] [#13487](#13487). * **File system** * The `fs.exists()` function now works correctly with `util.promisify()` [[`6e0eccd7a1`](6e0eccd7a1)] [#13316](#13316). * fs.Stats times are now also available as numbers [[`c756efb25a`](c756efb25a)] [#13173](#13173). * **Inspector** * It is now possible to bind to a random port using `--inspect=0` [[`cc6ec2fb27`](cc6ec2fb27)] [#5025](#5025). * **Zlib** * A regression in the Zlib module that made it impossible to properly subclasses `zlib.Deflate` and other Zlib classes has been fixed. [[`6aeb555cc4`](6aeb555cc4)] [#13374](#13374).

nodejs-github-bot added async_wrap c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 1, 2017

AndreasMadsen reviewed Jun 1, 2017

View reviewed changes

JiaLiPassion force-pushed the parent-promise branch from 28d4db5 to d721c6b Compare June 1, 2017 13:28

JiaLiPassion mentioned this pull request Jun 1, 2017

[WIP] feat(async): fix #740, use async hooks to wrap nodejs async api angular/zone.js#795

Open

jasnell approved these changes Jun 1, 2017

View reviewed changes

addaleax previously approved these changes Jun 1, 2017

View reviewed changes

addaleax approved these changes Jun 3, 2017

View reviewed changes

[squash] cleanup tests

b8f8f4b

AndreasMadsen force-pushed the parent-promise branch from ce12ab1 to b8f8f4b Compare June 3, 2017 11:09

AndreasMadsen approved these changes Jun 3, 2017

View reviewed changes

hayes mentioned this pull request Jun 3, 2017

async_hooks does not currently provide a way to track causality for a PromiseReactionJob #13437

Closed

addaleax closed this Jun 3, 2017

addaleax mentioned this pull request Jun 4, 2017

async_hooks: use resource objects for Promises #13452

Closed

4 tasks

jasnell mentioned this pull request Jun 5, 2017

8.1.0 Proposal #13483

Closed

gms1 mentioned this pull request Jun 9, 2017

async_hooks: not able to preserve continuation local storage inside Promises #13583

Closed

AndreasMadsen mentioned this pull request Jun 19, 2017

The dreaded "throwing after await" AndreasMadsen/trace#34

Open

AndreasMadsen mentioned this pull request Jul 17, 2017

Remove async_hooks runInAsyncIdScope API #14328

Closed

AndreasMadsen added the dont-land-on-v6.x label Jul 18, 2017

async_hooks: use parent promise as triggerId in init hook #13367

async_hooks: use parent promise as triggerId in init hook #13367

Conversation

JiaLiPassion commented Jun 1, 2017

Checklist

Affected core subsystem(s)

AndreasMadsen left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

JiaLiPassion commented Jun 1, 2017 • edited by AndreasMadsen Loading

AndreasMadsen commented Jun 1, 2017 • edited Loading

JiaLiPassion commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017 • edited Loading

JiaLiPassion commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiaLiPassion Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

AndreasMadsen commented Jun 1, 2017

addaleax commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

JiaLiPassion commented Jun 1, 2017 • edited by addaleax Loading

addaleax commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017 • edited Loading

addaleax commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

JiaLiPassion commented Jun 1, 2017

AndreasMadsen commented Jun 1, 2017

JiaLiPassion commented Jun 1, 2017 • edited by addaleax Loading

AndreasMadsen commented Jun 1, 2017

AndreasMadsen commented Jun 3, 2017

AndreasMadsen commented Jun 3, 2017 • edited Loading

Please keep this issue on track, this PR is just about fixing triggerId

JiaLiPassion commented Jun 3, 2017

addaleax commented Jun 3, 2017

JiaLiPassion commented Jun 3, 2017

addaleax commented Jun 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Jun 3, 2017

AndreasMadsen commented Jun 3, 2017

JiaLiPassion commented Jun 3, 2017 • edited Loading

AndreasMadsen commented Jun 3, 2017

JiaLiPassion commented Jun 3, 2017

AndreasMadsen left a comment

Choose a reason for hiding this comment

addaleax commented Jun 3, 2017 • edited Loading

JiaLiPassion commented Jun 3, 2017

AndreasMadsen left a comment •

edited

Loading

JiaLiPassion Jun 1, 2017 •

edited

Loading

JiaLiPassion commented Jun 1, 2017 •

edited by AndreasMadsen

Loading

AndreasMadsen commented Jun 1, 2017 •

edited

Loading

AndreasMadsen commented Jun 1, 2017 •

edited

Loading

JiaLiPassion Jun 1, 2017 •

edited

Loading

JiaLiPassion commented Jun 1, 2017 •

edited by addaleax

Loading

AndreasMadsen commented Jun 1, 2017 •

edited

Loading

JiaLiPassion commented Jun 1, 2017 •

edited by addaleax

Loading

AndreasMadsen commented Jun 3, 2017 •

edited

Loading

Please keep this issue on track, this PR is just about fixing `triggerId`

JiaLiPassion commented Jun 3, 2017 •

edited

Loading

addaleax commented Jun 3, 2017 •

edited

Loading