diff --git a/npm-packages/meteor-promise/fiber_pool.js b/npm-packages/meteor-promise/fiber_pool.js index 99565229b62..e2cfb29a77e 100644 --- a/npm-packages/meteor-promise/fiber_pool.js +++ b/npm-packages/meteor-promise/fiber_pool.js @@ -1,6 +1,13 @@ var assert = require("assert"); const { AsyncResource } = require("async_hooks"); +// Pool fibers are shared infrastructure: they must not inherit the AsyncLocalStorage stores +// (OpenTelemetry context, etc.) of whichever caller happened to create them, because the +// fiber's own async scope is what is active while it is parked between jobs. This resource is +// created at module load, before any request context exists, so anything created inside it +// starts with no stores at all. +const PRISTINE_ASYNC_SCOPE = new AsyncResource("FiberPoolRoot"); + function FiberPool(targetFiberCount) { assert.ok(this instanceof FiberPool); assert.strictEqual(typeof targetFiberCount, "number"); @@ -12,10 +19,13 @@ function FiberPool(targetFiberCount) { // with our processing of the callback queue. var originalYield = Fiber.yield; - var fiber = new Fiber(function () { + var fiber = PRISTINE_ASYNC_SCOPE.runInAsyncScope(() => new Fiber(function () { while (fiber) { - // Call Fiber.yield() to await further instructions. + // Call Fiber.yield() to await further instructions. Flag the fiber as idle so that + // instrumentation wrapping Fiber.yield can tell this park apart from a real yield. + fiber._meteorPromisePoolIdle = true; var entry = originalYield.call(Fiber); + delete fiber._meteorPromisePoolIdle; if (! (entry && typeof entry.callback === "function" && @@ -64,7 +74,7 @@ function FiberPool(targetFiberCount) { return; } } - }); + })); // Run the new Fiber up to the first yield point, so that it will be // ready to receive entries. diff --git a/npm-packages/meteor-promise/package.json b/npm-packages/meteor-promise/package.json index 90d783ca367..f9f049c4590 100644 --- a/npm-packages/meteor-promise/package.json +++ b/npm-packages/meteor-promise/package.json @@ -1,7 +1,7 @@ { "name": "meteor-promise", "author": "Ben Newman ", - "version": "0.9.1-2", + "version": "0.9.1-3", "description": "ES6 Promise polyfill with Fiber support", "keywords": [ "meteor", diff --git a/npm-packages/meteor-promise/test/tests.js b/npm-packages/meteor-promise/test/tests.js index 5fbb872121b..916fdf2dba6 100644 --- a/npm-packages/meteor-promise/test/tests.js +++ b/npm-packages/meteor-promise/test/tests.js @@ -409,3 +409,75 @@ describe("stack traces", function () { }); })); }); + +describe("fiber pool async context", function () { + var AsyncLocalStorage = require("async_hooks").AsyncLocalStorage; + var als = new AsyncLocalStorage(); + + // Pool fibers are shared between unrelated callers. Between jobs each pool fiber parks + // itself with Fiber.yield() in its own async scope, so that scope must not carry the + // AsyncLocalStorage stores of whichever caller happened to create the fiber. + // + // Only meaningful with native promises: the `promise` polyfill schedules reactions through + // asap, which batches unrelated callbacks under a single async context. + var itWithNativePromise = Promise === global.Promise ? it : it.skip; + itWithNativePromise("does not expose the creator's ALS store at the idle yield", function () { + var GROW = 30; // > targetFiberCount, so new pool fibers must be created under "creator" + var USE = 50; + var recording = false; + var seenAtYields = []; + var seenAtIdleYields = []; + var originalYield = Fiber.yield; + Fiber.yield = function () { + if (recording) { + seenAtYields.push(als.getStore()); + if (Fiber.current && Fiber.current._meteorPromisePoolIdle) { + seenAtIdleYields.push(als.getStore()); + } + } + return originalYield.apply(this, arguments); + }; + + return als.run("creator", function () { + return Promise.all(Array.from({ length: GROW }, function () { + return Promise.asyncApply(function () { Promise.await(wait(20)); }); + })); + }).then(function () { + recording = true; + return als.run("user", function () { + var chain = Promise.resolve(); + var seenInsideJobs = []; + for (var i = 0; i < USE; ++i) { + chain = chain.then(function () { + return Promise.asyncApply(function () { return als.getStore(); }); + }).then(function (store) { seenInsideJobs.push(store); }); + } + return chain.then(function () { return seenInsideJobs; }); + }); + }).then(function (seenInsideJobs) { + recording = false; + Fiber.yield = originalYield; + + // Inside a job the store is the submitter's. + assert.strictEqual(seenInsideJobs.length, USE); + seenInsideJobs.forEach(function (store) { assert.strictEqual(store, "user"); }); + + // No yield made while only "user" was running may see "creator". Before the fix every + // pool fiber's idle yield did, because the fiber inherited its creator's stores. + assert.ok(seenAtYields.length >= USE, "expected at least one yield per job, saw " + seenAtYields.length); + seenAtYields.forEach(function (store) { + assert.notStrictEqual(store, "creator", "a yield during the user phase saw the creator's store"); + }); + + // The pool flags its idle yield, and that yield carries no store at all. + assert.ok(seenAtIdleYields.length >= USE, "expected a flagged idle yield per job, saw " + seenAtIdleYields.length); + seenAtIdleYields.forEach(function (store) { + assert.strictEqual(store, undefined, "idle yield saw store " + JSON.stringify(store)); + }); + }, function (error) { + recording = false; + Fiber.yield = originalYield; + throw error; + }); + }); +}); diff --git a/packages/promise/package.js b/packages/promise/package.js index fbf4bc5cde1..daf8e72047a 100644 --- a/packages/promise/package.js +++ b/packages/promise/package.js @@ -1,6 +1,6 @@ Package.describe({ name: "promise", - version: "0.12.2", + version: "0.12.3", summary: "ECMAScript 2015 Promise polyfill with Fiber support", git: "https://github.com/meteor/promise", documentation: "README.md" @@ -8,7 +8,7 @@ Package.describe({ Npm.depends({ // TODO: can we just drop this now? - "meteor-promise": "0.9.1-2", + "meteor-promise": "0.9.1-3", "promise": "8.1.0" });