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 Variables resets value too early #13112

Open
zodern opened this issue Apr 26, 2024 · 2 comments
Open

[Meteor 3] Environment Variables resets value too early #13112

zodern opened this issue Apr 26, 2024 · 2 comments
Assignees
Milestone

Comments

@zodern
Copy link
Member

zodern commented Apr 26, 2024

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.

@zodern zodern changed the title [Meteor 3] Environment Variable [Meteor 3] Environment Variables reset value too early Apr 26, 2024
@zodern zodern changed the title [Meteor 3] Environment Variables reset value too early [Meteor 3] Environment Variables resets value too early Apr 26, 2024
@StorytellerCZ StorytellerCZ added this to the Release 3.0 milestone Apr 26, 2024
@nachocodoner
Copy link
Member

nachocodoner commented Apr 29, 2024

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

@zodern
Copy link
Member Author

zodern commented Apr 29, 2024

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.

@leonardoventurini leonardoventurini self-assigned this May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants