Description of feature
sigil.Execute exports every template variable into the process environment with os.Setenv, and restores the environment on the way out by snapshotting it at entry and replaying that snapshot through os.Clearenv (sigil.go:169-175,179-195 in v0.12.1). docket renders a recipe through it on every apply, plan, validate, export and loop iteration.
#514 added tasks.RenderTemplate, a mutex every render now goes through. That fixes the destructive half: two concurrent renders used to interleave a Clearenv with a restore, and because each replays the snapshot it took, variables were dropped from the process for good. With the command tests running in parallel, an unlocked render wiped the entire environment - PATH, HOME, everything - which is what the regression test in tasks/render_lock_test.go pins.
What the lock cannot fix is the transient window. Between a render's Clearenv and the end of its replay loop the process environment is empty, and a lock held by renders does nothing for a concurrent reader: os.Getenv is called all over docket and all over the standard library. So while docket is rendering a recipe, any other goroutine in the process that reads the environment, or spawns a child that inherits it, can see nothing.
For the CLI this is close to harmless - a run renders on one goroutine and nothing else is looking. It matters for #425, which is about making the export machinery usable from Go: an embedding caller that renders on one goroutine while doing anything environment-dependent on another has no way to know its environment is briefly empty, and no way to defend against it.
Two directions, neither small:
- Stop sigil touching the environment. Its
Execute uses the environment only to make $var visible to the POSIX preprocessor (mgood/go-posix); the template variables are already passed as a map. A fork or an upstream change that threads the lookup instead of exporting it would remove the hazard rather than narrow it.
- Render out of process. Heavy, but it makes the blast radius exactly the child.
Worth deciding before #425 promises anything about concurrent use.
Description of feature
sigil.Executeexports every template variable into the process environment withos.Setenv, and restores the environment on the way out by snapshotting it at entry and replaying that snapshot throughos.Clearenv(sigil.go:169-175,179-195in v0.12.1). docket renders a recipe through it on every apply, plan, validate, export and loop iteration.#514 added
tasks.RenderTemplate, a mutex every render now goes through. That fixes the destructive half: two concurrent renders used to interleave aClearenvwith a restore, and because each replays the snapshot it took, variables were dropped from the process for good. With the command tests running in parallel, an unlocked render wiped the entire environment -PATH,HOME, everything - which is what the regression test intasks/render_lock_test.gopins.What the lock cannot fix is the transient window. Between a render's
Clearenvand the end of its replay loop the process environment is empty, and a lock held by renders does nothing for a concurrent reader:os.Getenvis called all over docket and all over the standard library. So while docket is rendering a recipe, any other goroutine in the process that reads the environment, or spawns a child that inherits it, can see nothing.For the CLI this is close to harmless - a run renders on one goroutine and nothing else is looking. It matters for #425, which is about making the export machinery usable from Go: an embedding caller that renders on one goroutine while doing anything environment-dependent on another has no way to know its environment is briefly empty, and no way to defend against it.
Two directions, neither small:
Executeuses the environment only to make$varvisible to the POSIX preprocessor (mgood/go-posix); the template variables are already passed as a map. A fork or an upstream change that threads the lookup instead of exporting it would remove the hazard rather than narrow it.Worth deciding before #425 promises anything about concurrent use.