meteor/meteor

[Meteor 3] Environment Variables resets value too early

zodern opened this issue · 2 comments

With this code:

let ev1 = new Meteor.EnvironmentVariable();

async function runAsyncFunction() {
  await new Promise(resolve => setTimeout(resolve, 1000));
  console.log('value2', ev1.get());
}

ev1.withValue({ name: 'test' }, () => {
  runAsyncFunction();
  console.log('value 1', ev1.get());
});

In Meteor 1 and 2, it logs:

value 1 { name: 'test' }
value2 { name: 'test' }

However, in Meteor 3 it logs:

value 1 { name: 'test' }
value2 undefined

Meteor 1 and 2 preserve the value until all child async contexts complete. Async Local Storage does the same, which Environment Variables uses in Meteor 3, so it should be possible to preserve this behavior in Meteor 3.

These tests demonstrate the current behaviors and expected support in Meteor 3.0.

In your example, code might need adjustment to ensure you properly await children asyncs to maintain the env variable context. For instance:

let ev1 = new Meteor.EnvironmentVariable();

async function runAsyncFunction() {
  await new Promise(resolve => setTimeout(resolve, 1000));
  console.log('value2', ev1.get());
}

ev1.withValue({ name: 'test' }, () => {
  ➡️ await runAsyncFunction();
  console.log('value 1', ev1.get());
});

With this you get the proper response.

value 1 { name: 'test' }
value2 { name: 'test' }

I also tried it on the test we have.

While achieving this might be possible, given my limited familiarity with this code or Async Local Storage behavior, further exploration is needed. Otherwise, this change could be a breaking change caused by the way async operations behave in the new Meteor 3.x.

Thank you for bringing it to our attention.

cc @denihs

Preserving the env vars in async child contexts was an important part of the implementation in Meteor 1/2. Meteor wrapped most callback api's with Meteor.bindEnvironment, and had code in the promise package to preserve the env var values in async functions and promises. Since ALS works the same, it seems this behavior is generally expected.

You should be able to simply remove these lines. ALS makes the store available to all child resources, so if you don't mutate it it will work the same as in Meteor 1/2. There's no need to unset UPPER_CALL_DYNAMICS_KEY_NAME - you're creating a new store object each time, so outside code isn't able to read it (earlier implementations didn't create a new object, which is probably why the code was added).

When working on using ALS in montiapm:agent, I found it helpful in my understanding to read Node's source code for ALS.