From 05ad792a075046d66d43f1abcf1eefdd467e4061 Mon Sep 17 00:00:00 2001
From: James M Snell <jasnell@gmail.com>
Date: Fri, 27 Jan 2023 14:03:51 -0800
Subject: [PATCH] async_hooks: remove experimental onPropagate option
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The `onPropagate` option for `AsyncLocalStorage` is problematic for a
couple of reasons:

1. It is not expected to be forwards compatible in any way with the
   upcoming TC-39 `AsyncContext` proposal.
2. It introduces a non-trivial O(n) cost invoking a JavaScript callback
   for *every* AsyncResource that is created, including every Promise.

While it is still experimental, I recommend removing it while we can
revisit the fundamental use cases in light of the coming `AsyncContext`
proposal.

Refs: https://github.com/nodejs/node/issues/46374
PR-URL: https://github.com/nodejs/node/pull/46386
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
---
 doc/api/async_context.md                      | 24 +++------
 lib/async_hooks.js                            | 15 +-----
 ...st-async-local-storage-stop-propagation.js | 50 -------------------
 3 files changed, 9 insertions(+), 80 deletions(-)
 delete mode 100644 test/async-hooks/test-async-local-storage-stop-propagation.js

diff --git a/doc/api/async_context.md b/doc/api/async_context.md
index ce63e639f6ee59..94af6b016b1e74 100644
--- a/doc/api/async_context.md
+++ b/doc/api/async_context.md
@@ -116,35 +116,26 @@ Each instance of `AsyncLocalStorage` maintains an independent storage context.
 Multiple instances can safely exist simultaneously without risk of interfering
 with each other's data.
 
-### `new AsyncLocalStorage([options])`
+### `new AsyncLocalStorage()`
 
 <!-- YAML
 added:
  - v13.10.0
  - v12.17.0
 changes:
- - version: v19.2.0
+ - version: REPLACEME
+   pr-url: https://github.com/nodejs/node/pull/46386
+   description: Removed experimental onPropagate option.
+ - version:
+    - v19.2.0
+    - v18.13.0
    pr-url: https://github.com/nodejs/node/pull/45386
    description: Add option onPropagate.
 -->
 
-> Stability: 1 - `options.onPropagate` is experimental.
-
-* `options` {Object}
-  * `onPropagate` {Function} Optional callback invoked before a store is
-    propagated to a new async resource. Returning `true` allows propagation,
-    returning `false` avoids it. Default is to propagate always.
-
 Creates a new instance of `AsyncLocalStorage`. Store is only provided within a
 `run()` call or after an `enterWith()` call.
 
-The `onPropagate` is called during creation of an async resource. Throwing at
-this time will print the stack trace and exit. See
-[`async_hooks` Error handling][] for details.
-
-Creating an async resource within the `onPropagate` callback will result in
-a recursive call to `onPropagate`.
-
 ### `asyncLocalStorage.disable()`
 
 <!-- YAML
@@ -834,5 +825,4 @@ const server = createServer((req, res) => {
 [`EventEmitter`]: events.md#class-eventemitter
 [`Stream`]: stream.md#stream
 [`Worker`]: worker_threads.md#class-worker
-[`async_hooks` Error handling]: async_hooks.md#error-handling
 [`util.promisify()`]: util.md#utilpromisifyoriginal
diff --git a/lib/async_hooks.js b/lib/async_hooks.js
index bf76874ffe2c1b..525b69acbfc652 100644
--- a/lib/async_hooks.js
+++ b/lib/async_hooks.js
@@ -23,7 +23,6 @@ const {
 const { kEmptyObject } = require('internal/util');
 const {
   validateFunction,
-  validateObject,
   validateString,
 } = require('internal/validators');
 const internal_async_hooks = require('internal/async_hooks');
@@ -275,17 +274,9 @@ const storageHook = createHook({
 });
 
 class AsyncLocalStorage {
-  constructor(options = kEmptyObject) {
-    validateObject(options, 'options');
-
-    const { onPropagate = null } = options;
-    if (onPropagate !== null) {
-      validateFunction(onPropagate, 'options.onPropagate');
-    }
-
+  constructor() {
     this.kResourceStore = Symbol('kResourceStore');
     this.enabled = false;
-    this._onPropagate = onPropagate;
   }
 
   disable() {
@@ -312,9 +303,7 @@ class AsyncLocalStorage {
   _propagate(resource, triggerResource, type) {
     const store = triggerResource[this.kResourceStore];
     if (this.enabled) {
-      if (this._onPropagate === null || this._onPropagate(type, store)) {
-        resource[this.kResourceStore] = store;
-      }
+      resource[this.kResourceStore] = store;
     }
   }
 
diff --git a/test/async-hooks/test-async-local-storage-stop-propagation.js b/test/async-hooks/test-async-local-storage-stop-propagation.js
deleted file mode 100644
index 0480bd4be7f918..00000000000000
--- a/test/async-hooks/test-async-local-storage-stop-propagation.js
+++ /dev/null
@@ -1,50 +0,0 @@
-'use strict';
-const common = require('../common');
-const assert = require('assert');
-const { AsyncLocalStorage, AsyncResource } = require('async_hooks');
-
-let cnt = 0;
-function onPropagate(type, store) {
-  assert.strictEqual(als.getStore(), store);
-  cnt++;
-  if (cnt === 1) {
-    assert.strictEqual(type, 'r1');
-    return true;
-  }
-  if (cnt === 2) {
-    assert.strictEqual(type, 'r2');
-    return false;
-  }
-}
-
-const als = new AsyncLocalStorage({
-  onPropagate: common.mustCall(onPropagate, 2),
-});
-
-const myStore = {};
-
-als.run(myStore, common.mustCall(() => {
-  const r1 = new AsyncResource('r1');
-  const r2 = new AsyncResource('r2');
-  r1.runInAsyncScope(common.mustCall(() => {
-    assert.strictEqual(als.getStore(), myStore);
-  }));
-  r2.runInAsyncScope(common.mustCall(() => {
-    assert.strictEqual(als.getStore(), undefined);
-    r1.runInAsyncScope(common.mustCall(() => {
-      assert.strictEqual(als.getStore(), myStore);
-    }));
-  }));
-}));
-
-assert.throws(() => new AsyncLocalStorage(15), {
-  message: 'The "options" argument must be of type object. Received type number (15)',
-  code: 'ERR_INVALID_ARG_TYPE',
-  name: 'TypeError',
-});
-
-assert.throws(() => new AsyncLocalStorage({ onPropagate: 'bar' }), {
-  message: 'The "options.onPropagate" property must be of type function. Received type string (\'bar\')',
-  code: 'ERR_INVALID_ARG_TYPE',
-  name: 'TypeError',
-});