From f906610ca465ab6cf425604a37ff22992540556f Mon Sep 17 00:00:00 2001 From: rochdev Date: Tue, 1 Mar 2022 16:33:27 -0500 Subject: [PATCH 1/5] lib: fix AsyncResource.bind not using 'this' from the caller by default --- lib/async_hooks.js | 21 ++++++++++++--------- test/parallel/test-asyncresource-bind.js | 7 ++++++- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index af5a37b749d94b..3ef8922fd41553 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -223,15 +223,18 @@ class AsyncResource { return this[trigger_async_id_symbol]; } - bind(fn, thisArg = this) { + bind(fn, thisArg) { validateFunction(fn, 'fn'); - const ret = - FunctionPrototypeBind( - this.runInAsyncScope, - this, - fn, - thisArg); - ObjectDefineProperties(ret, { + const runInAsyncScope = FunctionPrototypeBind( + this.runInAsyncScope, + this, + fn); + function bound(...args) { + return runInAsyncScope( + thisArg !== undefined ? thisArg : this, + ...args); + } + ObjectDefineProperties(bound, { 'length': { configurable: true, enumerable: false, @@ -245,7 +248,7 @@ class AsyncResource { writable: true, } }); - return ret; + return bound; } static bind(fn, type, thisArg) { diff --git a/test/parallel/test-asyncresource-bind.js b/test/parallel/test-asyncresource-bind.js index a9f613d9302edf..29de9bbb0f10bb 100644 --- a/test/parallel/test-asyncresource-bind.js +++ b/test/parallel/test-asyncresource-bind.js @@ -41,7 +41,7 @@ const fn3 = asyncResource.bind(common.mustCall(function() { fn3(); const fn4 = asyncResource.bind(common.mustCall(function() { - assert.strictEqual(this, asyncResource); + assert.strictEqual(this, undefined); })); fn4(); @@ -49,3 +49,8 @@ const fn5 = asyncResource.bind(common.mustCall(function() { assert.strictEqual(this, false); }), false); fn5(); + +const fn6 = asyncResource.bind(common.mustCall(function() { + assert.strictEqual(this, 'test'); +})); +fn6.call('test'); From 280dbb2bfbabf5037ca1eea9566706624014ed0c Mon Sep 17 00:00:00 2001 From: rochdev Date: Tue, 1 Mar 2022 16:54:53 -0500 Subject: [PATCH 2/5] doc: update AsyncResource.bind doc to reflect new thisArg behaviour --- doc/api/async_context.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/api/async_context.md b/doc/api/async_context.md index 1194a313f45553..30d8cb6ae314c3 100644 --- a/doc/api/async_context.md +++ b/doc/api/async_context.md @@ -446,6 +446,9 @@ added: - v14.8.0 - v12.19.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/42177 + description: Changed the default when `thisArg` is not provided to use `this` from the caller. - version: v16.0.0 pr-url: https://github.com/nodejs/node/pull/36782 description: Added optional thisArg. @@ -468,6 +471,9 @@ added: - v14.8.0 - v12.19.0 changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/42177 + description: Changed the default when `thisArg` is not provided to use `this` from the caller. - version: v16.0.0 pr-url: https://github.com/nodejs/node/pull/36782 description: Added optional thisArg. From b28e3071d4306f849ec7b3c716a0437786817335 Mon Sep 17 00:00:00 2001 From: rochdev Date: Wed, 2 Mar 2022 19:27:44 -0500 Subject: [PATCH 3/5] lib: refactor AsyncResource.bind to not use globals --- lib/async_hooks.js | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 3ef8922fd41553..8862ae5b78d775 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -5,6 +5,7 @@ const { ArrayPrototypeIndexOf, ArrayPrototypePush, ArrayPrototypeSplice, + ArrayPrototypeUnshift, FunctionPrototypeBind, NumberIsSafeInteger, ObjectDefineProperties, @@ -225,14 +226,16 @@ class AsyncResource { bind(fn, thisArg) { validateFunction(fn, 'fn'); - const runInAsyncScope = FunctionPrototypeBind( - this.runInAsyncScope, - this, - fn); - function bound(...args) { - return runInAsyncScope( - thisArg !== undefined ? thisArg : this, - ...args); + const runInAsyncScope = this.runInAsyncScope; + const resource = this; + let bound; + if (thisArg === undefined) { + bound = function(...args) { + ArrayPrototypeUnshift(args, fn, this); + return ReflectApply(runInAsyncScope, resource, args); + }; + } else { + bound = FunctionPrototypeBind(runInAsyncScope, resource, fn, thisArg); } ObjectDefineProperties(bound, { 'length': { From c172ce29dee0566d2acb86c40240f383d3db7c46 Mon Sep 17 00:00:00 2001 From: rochdev Date: Thu, 3 Mar 2022 20:00:18 -0500 Subject: [PATCH 4/5] lib: refactor AsyncResource.bind to reduce number of allocations --- lib/async_hooks.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/lib/async_hooks.js b/lib/async_hooks.js index 8862ae5b78d775..6cc2052474d9c2 100644 --- a/lib/async_hooks.js +++ b/lib/async_hooks.js @@ -226,16 +226,15 @@ class AsyncResource { bind(fn, thisArg) { validateFunction(fn, 'fn'); - const runInAsyncScope = this.runInAsyncScope; - const resource = this; let bound; if (thisArg === undefined) { + const resource = this; bound = function(...args) { ArrayPrototypeUnshift(args, fn, this); - return ReflectApply(runInAsyncScope, resource, args); + return ReflectApply(resource.runInAsyncScope, resource, args); }; } else { - bound = FunctionPrototypeBind(runInAsyncScope, resource, fn, thisArg); + bound = FunctionPrototypeBind(this.runInAsyncScope, this, fn, thisArg); } ObjectDefineProperties(bound, { 'length': { From ab540ff6e3bcb086257a90d4a3a7442bc40607b4 Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Mon, 7 Mar 2022 23:42:29 +0100 Subject: [PATCH 5/5] Apply suggestions from code review --- doc/api/async_context.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/doc/api/async_context.md b/doc/api/async_context.md index 30d8cb6ae314c3..065aafd22181a6 100644 --- a/doc/api/async_context.md +++ b/doc/api/async_context.md @@ -448,7 +448,8 @@ added: changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/42177 - description: Changed the default when `thisArg` is not provided to use `this` from the caller. + description: Changed the default when `thisArg` is undefined to use `this` + from the caller. - version: v16.0.0 pr-url: https://github.com/nodejs/node/pull/36782 description: Added optional thisArg. @@ -473,7 +474,8 @@ added: changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/42177 - description: Changed the default when `thisArg` is not provided to use `this` from the caller. + description: Changed the default when `thisArg` is undefined to use `this` + from the caller. - version: v16.0.0 pr-url: https://github.com/nodejs/node/pull/36782 description: Added optional thisArg.