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

Environment leaks between spawned processes (on Windows) #1282

Closed
1 task done
tydeu opened this issue Jul 5, 2022 · 1 comment · Fixed by #1739
Closed
1 task done

Environment leaks between spawned processes (on Windows) #1282

tydeu opened this issue Jul 5, 2022 · 1 comment · Fixed by #1739
Labels
bug Something isn't working help wanted Extra attention is needed

Comments

@tydeu
Copy link
Member

tydeu commented Jul 5, 2022

Prerequisites

  • Put an X between the brackets on this line if you have done all of the following:
    • Checked that your issue isn't already filed.
    • Reduced the issue to a self-contained, reproducible test case.

Description

On Windows, spawning a process using IO.Process.spawn with one environment in one thread will affect the environment of another spawned process in another thread. This is a major problem for Lake which often wishes to spawn processes in different build threads with different environments.

This is due to this section of code (which specifically states that its manner of setting environment variables is thread-unsafe):

// TODO(gabriel): this is thread-unsafe
std::unordered_map<std::string, optional<std::string>> old_env_vars;
for (auto & entry : env) {
optional<std::string> old;
if (auto old_val = getenv(entry.fst().data()))
old = std::string(old_val);
old_env_vars[entry.fst().to_std_string()] = old;
if (entry.snd()) {
SetEnvironmentVariable(entry.fst().data(), entry.snd().get()->data());
} else {
SetEnvironmentVariable(entry.fst().data(), nullptr);
}
}

Based on the comment, I assume this may be an issue for @gebner?

Steps to Reproduce

def printenv (var : String) (env : Array (String × Option String) := #[]) : IO String :=
  IO.Process.output {cmd := "printenv", args :=  #[var], env} <&> (·.stdout)

def singleTest : IO PUnit := do
  IO.println s!"Initial: {repr (← printenv "LEAN_CC").trim}"

def jointSyncTest : IO PUnit := do
  let a ← printenv "LEAN_CC" #[("LEAN_CC", "cc")]
  let b ← printenv "LEAN_CC" #[]
  IO.print s!"1st: {repr a.trim}; "
  IO.println s!"2nd: {repr b.trim}"

def jointAsyncTest : IO PUnit := do
  let t1 ← IO.asTask <| printenv "LEAN_CC" #[("LEAN_CC", "cc")]
  let t2 ← IO.asTask <| printenv "LEAN_CC" #[]
  IO.print s!"Task 1: {repr (← IO.ofExcept (← IO.wait t1)).trim}; "
  IO.println s!"Task 2: {repr (← IO.ofExcept (← IO.wait t2)).trim}"

#eval singleTest -- Initial: ""
#eval jointSyncTest -- 1st: "cc"; 2nd: ""
#eval jointAsyncTest -- Task 1: "cc"; Task 2: "cc"

Expected behavior:

Environment does not leak between separately spawned process (even those spawned in parallel).

Actual behavior:

Environment leaks between processes spawned in parallel.

Reproduces how often:

Always (on Windows).

Versions

Windows 20H2
Lean (version 4.0.0-nightly-2022-07-03, commit 7326c817d22e, Release)

Additional Information

This bug can exacerbate the issue in #1281. If one process has LEAN_CC set and leanc is run in separate, parallel process, leanc breaks. I discovered this when attempting to get Alloy to work with the new build features of Lake. The environment set for the alloy executable (which included a set LEAN_CC) would break parallel runs of leanc.

@gebner
Copy link
Member

gebner commented Jul 5, 2022

Based on the comment, I assume this may be an issue for @gebner?

The comment just means that this has been on (the end of) my todo list for five years. 😄

From what I can tell, the windows CreateProcess function actually takes an argument for the environment variables. We'd just need to pass the right thing there. I don't have a windows machine, so I can't easily debug this.

@leodemoura leodemoura added the bug Something isn't working label Jul 6, 2022
@leodemoura leodemoura added the help wanted Extra attention is needed label Jul 28, 2022
awson added a commit to awson/lean4 that referenced this issue Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants