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

async_hooks: add currentResource #21313

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
165 changes: 165 additions & 0 deletions benchmark/async_hooks/current-resource-vs-destroy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
'use strict';

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO some comments explaining what this benchmark is testing will help readers going forward.


const { promisify } = require('util');
const { readFile } = require('fs');
const sleep = promisify(setTimeout);
const read = promisify(readFile);
const common = require('../common.js');
const {
createHook,
currentResource,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be named executionAsyncResource for consistency?

executionAsyncId
} = require('async_hooks');
const { createServer } = require('http');

// configuration for the http server
// there is no need for parameters in this test
const connections = 500;
const path = '/';

const bench = common.createBenchmark(main, {
type: ['current-resource', 'destroy'],
method: ['callbacks', 'async'],
n: [1e6]
});

function buildCurrentResource(getServe) {
const server = createServer(getServe(getCLS, setCLS));
const hook = createHook({ init });
const cls = Symbol('cls');
let closed = false;
hook.enable();

return {
server,
close
};

function getCLS() {
// we need to protect this, as once the hook is
// disabled currentResource will return null
if (closed) {
return;
}

const resource = currentResource();
if (!resource[cls]) {
return null;
}
return resource[cls].state;
}

function setCLS(state) {
// we need to protect this, as once the hook is
// disabled currentResource will return null
if (closed) {
return;
}
const resource = currentResource();
if (!resource[cls]) {
resource[cls] = { state };
} else {
resource[cls].state = state;
}
}

function init(asyncId, type, triggerAsyncId, resource) {
if (type === 'TIMERWRAP') return;

var cr = currentResource();
if (cr) {
resource[cls] = cr[cls];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this necessarily pertinent to the intent of the benchmark here, but I'll call it out nonetheless: If I'm reading correctly, this results in every resource sharing the same object instance for cls, and every setCLS call will just overwrite the state field. For correctness here, each resource needs a copy (e.g., resource[cls] = {...cr[cls]}), or you need to maintain a "list to the root" (e.g., resource[cls] = {props:{}, parent: cr[cls]}.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per conversation, here's an example of where values would be over-written:

function f0() {
  setCLS("a");
  setTimeout(function f1() {
     setCLS("b");
     setTimeout(function f2() {
        getCLS(); // expect "b", will get "c"
     }, 100);

  }, 10);

  setTimeout(function f3() => {
      getCLS(); // expect "a", will get "b"
      setCLS("c");
      setTimeout(function f4() {
          getCLS(); // get "c"
      }, 25);
  }, 25);
};
f();

Order of execution is f0, f1, f3, f4, f2.

But f0->f1->f2 is one path of the tree, and another indepdent path is f0->f3->f4. Values visible on one path should not be visible on the path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think branch containment really matters for this test. We just need to verify that an object can be passed through the tree via attachment to the current resource in some form.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think branch containment really matters for this test.

Agreed, but it came up on the diag call & I offered to post example code. :)

}
}

function close() {
closed = true;
hook.disable();
server.close();
}
}

function buildDestroy(getServe) {
const transactions = new Map();
const server = createServer(getServe(getCLS, setCLS));
const hook = createHook({ init, destroy });
hook.enable();

return {
server,
close
};

function getCLS() {
const asyncId = executionAsyncId();
return transactions.has(asyncId) ? transactions.get(asyncId) : null;
}

function setCLS(value) {
const asyncId = executionAsyncId();
transactions.set(asyncId, value);
}

function init(asyncId, type, triggerAsyncId, resource) {
if (type === 'TIMERWRAP') return;

transactions.set(asyncId, getCLS());
}

function destroy(asyncId) {
transactions.delete(asyncId);
}

function close() {
hook.disable();
server.close();
}
}

function getServeAwait(getCLS, setCLS) {
return async function serve(req, res) {
setCLS(Math.random());
await sleep(10);
await read(__filename);
res.setHeader('content-type', 'application/json');
res.end(JSON.stringify({ cls: getCLS() }));
};
}

function getServeCallbacks(getCLS, setCLS) {
return function serve(req, res) {
setCLS(Math.random());
setTimeout(function() {
readFile(__filename, function() {
res.setHeader('content-type', 'application/json');
res.end(JSON.stringify({ cls: getCLS() }));
});
}, 10);
};
}

const types = {
'current-resource': buildCurrentResource,
'destroy': buildDestroy
};

const asyncMethod = {
'callbacks': getServeCallbacks,
'async': getServeAwait
};

function main({ type, method }) {
const { server, close } = types[type](asyncMethod[method]);

server
.listen(common.PORT)
.on('listening', function() {

bench.http({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer a benchmark that doesn't use http. It'll make it hard to notice incremental improvements to this code because the spread and overhead of http is so high.

path,
connections
}, function() {
close();
});
});
}
51 changes: 51 additions & 0 deletions doc/api/async_hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -462,6 +462,57 @@ init for PROMISE with id 6, trigger id: 5 # the Promise returned by then()
after 6
```

#### async_hooks.currentResource()

<!-- YAML
added: REPLACEME
-->

* Returns: {Object} The resource that triggered the current
execution context.
Useful to store data within the resource.

```js
const { open } = require('fs');
const { executionAsyncId, currentResource } = require('async_hooks');

console.log(executionAsyncId(), currentResource()); // 1 null
open(__filename, 'r', (err, fd) => {
console.log(executionAsyncId(), currentResource()); // 7 FSReqWrap
});
```

This can be used to implement continuation local storage without the
using of a tracking `Map` to store the metadata:

```js
const { createServer } = require('http');
const {
executionAsyncId,
currentResource,
createHook
} = require('async_hooks');
const sym = Symbol('state'); // private symbol to avoid pollution

createHook({
init(asyncId, type, triggerAsyncId, resource) {
const cr = currentResource();
if (cr) {
resource[sym] = cr[sym];
}
}
}).enable();

const server = createServer(function(req, res) {
currentResource()[sym] = { state: req.url };
setTimeout(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function() { -> () => {?

res.end(JSON.stringify(currentResource()[sym]));
}, 100);
}).listen(3000);
```

`currentResource()` will return `null` during application bootstrap.

#### async_hooks.executionAsyncId()

<!-- YAML
Expand Down
2 changes: 2 additions & 0 deletions lib/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const {
emitBefore,
emitAfter,
emitDestroy,
currentResource
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, how about using executionAsyncResource?

} = internal_async_hooks;

// Get symbols
Expand Down Expand Up @@ -216,6 +217,7 @@ module.exports = {
createHook,
executionAsyncId,
triggerAsyncId,
currentResource,
// Embedder API
AsyncResource,
};
9 changes: 8 additions & 1 deletion lib/internal/async_hooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ const active_hooks = {

const { registerDestroyHook } = async_wrap;

const { currentResource, setCurrentResource } = async_wrap;

// Each constant tracks how many callbacks there are for any given step of
// async execution. These are tracked so if the user didn't include callbacks
// for a given step, that step can bail out early.
Expand Down Expand Up @@ -337,13 +339,14 @@ function emitInitScript(asyncId, type, triggerAsyncId, resource) {
}


function emitBeforeScript(asyncId, triggerAsyncId) {
function emitBeforeScript(asyncId, triggerAsyncId, resource) {
// Validate the ids. An id of -1 means it was never set and is visible on the
// call graph. An id < -1 should never happen in any circumstance. Throw
// on user calls because async state should still be recoverable.
validateAsyncId(asyncId, 'asyncId');
validateAsyncId(triggerAsyncId, 'triggerAsyncId');

setCurrentResource(resource);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

assert here is that currentResource === undefined || currentResource === null.

pushAsyncIds(asyncId, triggerAsyncId);

if (async_hook_fields[kBefore] > 0)
Expand All @@ -357,6 +360,8 @@ function emitAfterScript(asyncId) {
if (async_hook_fields[kAfter] > 0)
emitAfterNative(asyncId);

setCurrentResource(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have nested emitBefore(…, res1)emitBefore(…, res2)emitAfter(…)emitAfter(…), then after the first emitAfter we want the current resource to be res1

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, shouldn't setCurrentResource be pushCurrentResource/popCurrentResource?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have nested emitBefore(…, res1) – emitBefore(…, res2) – emitAfter(…) – emitAfter(…), then after the first emitAfter we want the current resource to be res1

I thought the same. However I struggled to execute user code after emitAfter. Can you add an example?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should work:

const async_hooks = require("async_hooks");
const fs = require("fs");

let indent = 0;

function init(asyncId, type, triggerAsyncId, resource) {
  const eid = async_hooks.executionAsyncId();
  const indentStr = ' '.repeat(indent);
  fs.writeSync(1, `${indentStr}${type}(${asyncId}): trigger: ${triggerAsyncId} execution: ${eid}\n`);
}

function before(asyncId) {
  const indentStr = ' '.repeat(indent);
  fs.writeSync(1, `${indentStr}before:  ${asyncId}\n`);
  indent += 2;
}

function after(asyncId) {
  indent -= 2;
  const indentStr = ' '.repeat(indent);
  fs.writeSync(1, `${indentStr}after:   ${asyncId}\n`);
}

function destroy(asyncId) {
  const indentStr = ' '.repeat(indent);
  fs.writeSync(1, `${indentStr}destroy: ${asyncId}\n`);
}

async_hooks.createHook({ init, before, after, destroy }).enable();

class MyResource extends async_hooks.AsyncResource {
  constructor() {
    super("XXX");
  }
}
const res = new MyResource();

setImmediate(() => {
  const indentStr = ' '.repeat(indent);
  fs.writeSync(1, `${indentStr}between before\n`);
  res.runInAsyncScope(() => {
    const indentStr = ' '.repeat(indent);
    fs.writeSync(1, `${indentStr}inner \n`);
  });
  fs.writeSync(1, `${indentStr}between after\n`);
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this only happening with runInAsyncScope? That would be easy to fix without adding a full stack where we push/pop.

Adding a full stack is also a serious refactoring of the async_hooks machinery, something that this PR currently avoids.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mcollina We do have a full stack in place currently, though?

Is this only happening with runInAsyncScope?

It can happen with all mechanisms that trigger async hooks.


popAsyncIds(asyncId);
}

Expand Down Expand Up @@ -444,6 +449,7 @@ module.exports = {
clearDefaultTriggerAsyncId,
clearAsyncIdStack,
hasAsyncIdStack,
currentResource,
// Internal Embedder API
newAsyncId,
getOrSetAsyncId,
Expand All @@ -457,4 +463,5 @@ module.exports = {
emitAfter: emitAfterScript,
emitDestroy: emitDestroyScript,
registerDestroyHook,
setCurrentResource
};
2 changes: 1 addition & 1 deletion lib/internal/process/next_tick.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ function setupNextTick(_setupNextTick, _setupPromises) {
do {
while (tock = queue.shift()) {
const asyncId = tock[async_id_symbol];
emitBefore(asyncId, tock[trigger_async_id_symbol]);
emitBefore(asyncId, tock[trigger_async_id_symbol], tock);
// emitDestroy() places the async_id_symbol into an asynchronous queue
// that calls the destroy callback in the future. It's called before
// calling tock.callback so destroy will be called even if the callback
Expand Down
8 changes: 5 additions & 3 deletions lib/timers.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@ const {
// The needed emit*() functions.
emitBefore,
emitAfter,
emitDestroy
emitDestroy,
setCurrentResource
} = require('internal/async_hooks');

// *Must* match Environment::ImmediateInfo::Fields in src/env.h.
Expand Down Expand Up @@ -310,7 +311,7 @@ function listOnTimeout(list, now) {
continue;
}

emitBefore(asyncId, timer[trigger_async_id_symbol]);
emitBefore(asyncId, timer[trigger_async_id_symbol], timer);

tryOnTimeout(timer);

Expand Down Expand Up @@ -340,6 +341,7 @@ function tryOnTimeout(timer, start) {
try {
ontimeout(timer);
} finally {
setCurrentResource(null);
if (timer._repeat) {
rearm(timer, start);
} else {
Expand Down Expand Up @@ -621,7 +623,7 @@ function processImmediate() {
immediate._destroyed = true;

const asyncId = immediate[async_id_symbol];
emitBefore(asyncId, immediate[trigger_async_id_symbol]);
emitBefore(asyncId, immediate[trigger_async_id_symbol], immediate);

count++;
if (immediate[kRefed])
Expand Down
2 changes: 1 addition & 1 deletion src/async_wrap-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ inline AsyncWrap::AsyncScope::AsyncScope(AsyncWrap* wrap)
Environment* env = wrap->env();
if (env->async_hooks()->fields()[Environment::AsyncHooks::kBefore] == 0)
return;
EmitBefore(env, wrap->get_async_id());
EmitBefore(env, wrap->get_async_id(), wrap->object());
}

inline AsyncWrap::AsyncScope::~AsyncScope() {
Expand Down
Loading