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

[Meteor 3] Environment Variable Tweaks #13128

Merged
merged 30 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
5acf2c5
no need to unset value manually since als already manages it well -- …
leonardoventurini May 7, 2024
c6df7cb
make it simpler to run tests in async functions
leonardoventurini May 8, 2024
3c82f87
add async/await version of test
leonardoventurini May 8, 2024
85c4c68
add commands
leonardoventurini May 8, 2024
b544f3c
apparently there is no need for this
leonardoventurini May 9, 2024
08c2d7b
make sure there is consistent ev value across client/server
leonardoventurini May 9, 2024
ce37839
Merge branch 'refs/heads/release-3.0' into feature/ev-improvements
leonardoventurini May 9, 2024
d2d7386
use old syntax
leonardoventurini May 9, 2024
6822f65
fix syntax
leonardoventurini May 9, 2024
e6eb9ed
simplify code
leonardoventurini May 9, 2024
e783309
add command
leonardoventurini May 10, 2024
58a1fcb
simplify tests and add comment
leonardoventurini May 10, 2024
0116eb2
use then instead of finally for better compatibility with legacy runt…
leonardoventurini May 13, 2024
75221de
add test
leonardoventurini May 13, 2024
a8edea4
add helper
leonardoventurini May 13, 2024
e7d8d32
run test function in fresh environment
leonardoventurini May 13, 2024
462b48e
Revert "run test function in fresh environment"
leonardoventurini May 13, 2024
52712ee
run specific test in fresh environment instead
leonardoventurini May 13, 2024
706a4af
add commands
leonardoventurini May 13, 2024
aedb02c
commit shrinkwrap
leonardoventurini May 13, 2024
92b550f
Merge branch 'release-3.0' into feature/ev-improvements
leonardoventurini May 14, 2024
b7ab641
add rethrow
leonardoventurini May 13, 2024
d27946d
implement suggestion
leonardoventurini May 15, 2024
67c3850
Merge branch 'release-3.0' into feature/ev-improvements
leonardoventurini May 16, 2024
525df1a
bump version manually
leonardoventurini May 20, 2024
307029b
adjust cmd
leonardoventurini May 20, 2024
3696e85
Merge branch 'refs/heads/release-3.0' into feature/ev-improvements
leonardoventurini May 20, 2024
3b77370
Merge branch 'release-3.0' into feature/ev-improvements
leonardoventurini May 20, 2024
1e3a2b0
Merge branch 'release-3.0' into feature/ev-improvements
leonardoventurini May 20, 2024
e413a04
Merge branch 'release-3.0' into feature/ev-improvements
leonardoventurini May 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 32 additions & 0 deletions .envrc
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
#!/bin/env zsh

#
# Commands and shortcuts for Meteor core development, you can load these in your terminal by running `source .envrc`.
# Or by adding `[[ -s .envrc ]] && source .envrc` to your `.zshrc` or `.bashrc`.
#

export ROOT_DIR=$(git rev-parse --show-toplevel)

function @meteor {
"$ROOT_DIR/meteor" "$@"
}

function @test-package {
@meteor test-packages "$@" --exclude-archs=web.browser.legacy,web.cordova
}

function @test-packages {
TINYTEST_FILTER="$1" @meteor test-packages --exclude-archs=web.browser.legacy,web.cordova
}

function @test-self {
@meteor self-test "$@"
}

function @check-syntax {
node "$ROOT_DIR/scripts/admin/check-legacy-syntax/check-syntax.js"
}

function @generate-dev-bundle {
"$ROOT_DIR/scripts/generate-dev-bundle.sh"
}
nachocodoner marked this conversation as resolved.
Show resolved Hide resolved
1 change: 0 additions & 1 deletion packages/accounts-password/password_tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -1213,7 +1213,6 @@ if (Meteor.isServer) (() => {
// This test properly belongs in accounts-base/accounts_tests.js, but
// this is where the tests that actually log in are.
Tinytest.addAsync('accounts - user() out of context', async test => {
// basic server context, no method.
await test.throwsAsync(
async () =>
await Meteor.user()
Expand Down
4 changes: 4 additions & 0 deletions packages/meteor/asl-helpers-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,7 @@ Meteor.isFibersDisabled = true;
Meteor._isPromise = function (r) {
return r && typeof r.then === "function";
};

Meteor._runFresh = function (fn) {
return fn();
};
6 changes: 6 additions & 0 deletions packages/meteor/asl-helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,10 +36,16 @@ function updateAslStore(key, value) {
return getAslStore()[key] = value;
}

function runFresh(fn) {
var asyncLocalStorage = getAsl();
return asyncLocalStorage.run({}, fn);
}

Meteor._getAsl = getAsl;
Meteor._getAslStore = getAslStore;
Meteor._getValueFromAslStore = getValueFromAslStore;
Meteor._updateAslStore = updateAslStore;
Meteor._runFresh = runFresh;

Meteor._runAsync = function (fn, ctx, store) {
if (store === undefined) {
Expand Down
2 changes: 1 addition & 1 deletion packages/meteor/async_helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ Object.assign(AsynchronousQueue.prototype, {
const promise = new Promise(r => resolve = r);
const runImmediateHandle = (fn) => {
if (Meteor.isServer) {
setImmediate(fn);
Meteor._runFresh(() => setImmediate(fn))
return;
}
setTimeout(fn, 0);
Expand Down
17 changes: 16 additions & 1 deletion packages/meteor/dynamics_browser.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,26 @@ EVp.getOrNullIfOutsideFiber = function () {
*/
EVp.withValue = function (value, func) {
var saved = currentValues[this.slot];

try {
currentValues[this.slot] = value;

var ret = func();
var isPromise = Meteor._isPromise(ret);

if (isPromise) {
var self = this;
return ret.then(function (res) {
currentValues[self.slot] = saved;
return res
});
}
} catch (e) {
throw e;
} finally {
zodern marked this conversation as resolved.
Show resolved Hide resolved
currentValues[this.slot] = saved;
if (!isPromise) {
currentValues[this.slot] = saved;
}
}

return ret;
Expand Down
22 changes: 6 additions & 16 deletions packages/meteor/dynamics_nodejs.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,20 +60,10 @@ class EnvironmentVariableAsync {
}

return Meteor._runAsync(
async function () {
let ret;
try {
Meteor._updateAslStore(CURRENT_VALUE_KEY_NAME, value);
Meteor._updateAslStore(UPPER_CALL_DYNAMICS_KEY_NAME, dynamics);
ret = await func();
} finally {
Meteor._updateAslStore(CURRENT_VALUE_KEY_NAME, undefined);
Meteor._updateAslStore(
UPPER_CALL_DYNAMICS_KEY_NAME,
undefined,
);
}
return ret;
function () {
Meteor._updateAslStore(CURRENT_VALUE_KEY_NAME, value);
Meteor._updateAslStore(UPPER_CALL_DYNAMICS_KEY_NAME, dynamics);
return func();
Copy link
Contributor

@denihs denihs May 10, 2024

Choose a reason for hiding this comment

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

I think this will not work.

We need to wait for the func() to finish and reset the states of CURRENT_VALUE_KEY_NAME and UPPER_CALL_DYNAMICS_KEY_NAME after the func() is done.

This is because you can call a withValues inside another, and you need to set and reset those states to keep context between calls.

Otherwise, you'll get issues like this: #13063

Copy link
Member

Choose a reason for hiding this comment

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

#13063 changed it to create a new dynamics object each time instead of mutating the parent dynamics. This should remove the need to reset the state since the parent and children all have their own state. If you comment out the reset code, the example code from the PR description stills works correctly.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with @zodern, I will include that code as a test just to make sure.

},
self,
Object.assign(
Expand Down Expand Up @@ -142,10 +132,10 @@ Meteor.EnvironmentVariable = EnvironmentVariableAsync;
/**
* @summary Stores the current Meteor environment variables, and wraps the
* function to run with the environment variables restored. On the server, the
* function is wrapped within a fiber.
* function is wrapped within Async Local Storage.
*
* This function has two reasons:
* 1. Return the function to be executed on the MeteorJS context, having it assinged in the async localstorage.
* 1. Return the function to be executed on the MeteorJS context, having it assigned in Async Local Storage.
* 2. Better error handling, the error message will be more clear.
* @locus Anywhere
* @memberOf Meteor
Expand Down
57 changes: 54 additions & 3 deletions packages/meteor/dynamics_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,7 @@ Tinytest.addAsync("environment - bindEnvironment", async function (test) {
test.equal(raised_f, null);

test.equal(await f(true), undefined);
test.equal(raised_f, "test");
test.equal(raised_f, "test", 'raised_f should be "test"');
};

// At top level
Expand All @@ -117,9 +117,9 @@ Tinytest.addAsync("environment - bindEnvironment", async function (test) {

// Inside a withValue

await CurrentFoo.withValue(22, function () {
await CurrentFoo.withValue(22, async function () {
test.equal(CurrentFoo.get(), 22);
test_f();
await test_f();
test.equal(CurrentFoo.get(), 22);
});

Expand Down Expand Up @@ -178,3 +178,54 @@ Tinytest.addAsync("environment - bare bindEnvironment",
setTimeout(f, 0);
});
});

/**
* This won't work on the client due to the absence of ALS/AH
*/
if (Meteor.isServer) {
Tinytest.addAsync('environment - preserve ev value async/await', async function (test) {
let val1 = null;
let val2 = null;

let ev1 = new Meteor.EnvironmentVariable();

async function runAsyncFunction() {
await test.sleep(10)
val2 = ev1.get();
}

ev1.withValue({ name: 'test' }, async () => {
runAsyncFunction();

val1 = ev1.get();
});

await test.sleep(20)

test.equal(val1, { name: 'test' }, 'val1 should be equal to { name: "test" }');
test.equal(val2, { name: 'test' }, 'val2 should be equal to val1');
})

Tinytest.addAsync('environment - should not access ev after it finishes', async function (test) {
const context1 = new Meteor.EnvironmentVariable();
const context2 = new Meteor.EnvironmentVariable();

await context1.withValue({ idd: 123 }, async () => {
await context2.withValue({ idd: 456 }, async () => {
await context2.withValue({ idd: 789 }, async () => {
test.equal(context2.get(), { idd: 789 }, 'context2 should be 789');
})
test.equal(context2.get(), { idd: 456 }, 'context2 should be 456');
})

test.equal(context1.get(), { idd: 123 }, 'context1 should be 123');
test.equal(context2.get(), undefined, 'context2 should be undefined');
});
})
}

Tinytest.add('environment - consistent ev value', function (test) {
let ev1 = new Meteor.EnvironmentVariable();
const ret = ev1.withValue(10, () => 5);
test.equal(ret, 5);
})
4 changes: 4 additions & 0 deletions packages/tinytest/tinytest.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ export class TestCaseResults {
this.extraDetails = {};
}

sleep(ms = 0) {
return new Promise(resolve => setTimeout(resolve, ms));
}

ok(doc) {
var ok = {type: "ok"};
if (doc)
Expand Down