os: sharing of environment variables in Plan 9 doesn't match go expectations

[millerresearch]

What version of Go are you using (go version)?

go version go1.10 plan9/386

Does this issue reproduce with the latest release?

yes

What did you do?

 go test -run env -count 10000 os

What did you expect to see?

"ok"

What did you see instead?

--- FAIL: TestUnsetenv (0.01s)
	env_test.go:88: Setenv didn't set TestUnsetenv
--- FAIL: ExampleGetenv (0.00s)
got:
lives in .
want:
gopher lives in /usr/gopher.
FAIL
FAIL	os	103.120s

Environment variables in Plan 9 are implemented by the in-kernel /env device which by default is shared between parent and child processes (and therefore between sibling processes as well). The Plan 9 versions of syscall.Getenv and syscall.Setenv read and write the /env device directly, so concurrent accesses to environment variables from separate Go programs may interfere with each other. Go documentation doesn't seem to rule out sharing of environment variables explicitly, but that may be because it doesn't happen in most (any?) other operating systems.

I propose that the Plan 9 Go library should keep an internal cache of environment variables in the style of syscall/env_unix.go, to make the semantics more like other OSes (and to correct the test failures above). The cache will also avoid the extra system calls which the present implementation requires for each variable access.

Plan 9 developers who want the (non-portable) sharing semantics of environment variables will still be able to use direct I/O to the /env device for this purpose.

If nobody objects, I have a CL ready.

[bradfitz]

Sounds fine. This is even okay for Go 1.11, assuming it only touches Plan 9 code.

[cherrymui]

I don't know the history, but why don't we have RFENVG set for fork (by default)?

[millerresearch]

why don't we have RFENVG set for fork (by default)?

Good question. It was before my time too; maybe @rsc remembers?

But even with RFENVG, I think it would still be worth cacheing the environment to avoid the cost of reading the /env directory and going through open+seek(twice)+read+close syscalls for each variable, every time os.Environ is called.

[rminnich]

Don't do these changes. Skip the test. We want a working Getenv/Setenv that works on plan 9, not a version that breaks Plan 9 everywhere. Speaking personally, I'm not concerned about making Plan 9 match Unix env behavior -- we had that and moved on from it. Why regress?

In Plan 9, env is a kernel driver. Last time I measured it, in 2002, on a 300 mhz. geode, getenv took 30 microseconds or so. yep, slow, but it was nowhere near a problem. If you're reading 30,000 environment variables more than once a second, you have a different problem :-)

There are lots of benefits in having /env in your name space, not on your stack. I can, e.g., untar a file into /env and set my namespace that way. This was used back in the day to transparently move desktops between plan 9 systems, e.g., you could tar and untar your desktop on a new system.

I can use any program that reads and writes files to read and write env variables.

These are compelling properties, but, more important, they require two things:

• don't set RFENVG on fork, because then I can't run a process that changes /env
• don't cache /env, because then I can't see the output of such a command.

This test is wrong for plan 9 and should be skipped on any plan 9 system. We'll need to make sure the harvey port skips it too.

Of course, this is is Richard Miller I'm disagreeing with, so I'm happy to admit I'm wrong :-)

[bradfitz]

@rminnich, for better or worse, the Go packages (outside of syscall) are generally expected to behave with Unix semantics. That means Windows and Plan 9 need to bend a bit.

If we don't make changes like this, fewer Go programs work by default on Plan 9.

If you want Plan 9 semantics, you can use the syscall package directly, or os.Open/OpenFile any Plan 9 file (/env/*) you want.

[millerresearch]

What about making Setenv/Getenv in x/sys/plan9 give the shared environment semantics using the /env device, while letting Setenv/Getenv in os and syscall behave more like Unix for portability?

Looking into the history, I see that Setenv/Getenv in x/sys/plan9 at one time did exactly what I'm proposing for Unix-like behaviour (ie cache the values in a local map), but CL 10404 changed it to call the syscall functions giving Plan 9 semantics instead. Maybe I don't understand the subtleties of x/sys vs syscall, but I thought the expectation was for platform dependent things to be in x/sys, while syscall (and os even more so) tries to offer portable behaviour. Is it a bad thing to offer a choice of plan9.Setenv and os.Setenv with different semantics?

I think @rminnich is right about performance; I'm just overly sensitive because my desktop machine is a Raspberry Pi.

[bradfitz]

syscall is low-level non-portable stuff, but we froze syscall because we didn't want it public or changing all the time or part of the standard library.

So we copied syscall to x/sys. Think of syscall and x/sys as identical (API, semantics) but we only add to x/sys these days. In Go 2 we probably won't have any public syscall package.

[bcmills]

What about making Setenv/Getenv in x/sys/plan9 give the shared environment semantics using the /env device, while letting Setenv/Getenv in os and syscall behave more like Unix for portability?

To the extent that syscall exists at all, it can and should be system-specific.

os, on the other hand, should be portable.

[fhs]

For reference, commit 1583931 added environment variable caching, and then commit 70896a7 removed it. The reason for removing caching seems to be that it was being done inconsistently. Also, from the CL discussion:

Btw, rc also caches variables, though the reasons why are probably
not relevant Go.

Yes, I think it's a better approach to keep the behavior
of the Go libraries unsurprising and to let the Go programs
do the cache themselves when they really need.

The standard library seems to be very liberal about calling os.Environ, which results in a lot of syscalls in Plan 9. I'd be curious how much faster make.rc gets with some kind of caching.

I think a bigger issue is that the os package is not doing rfork(RFENVG) (see how it breaks cmd/go in issue #34971). I propose adding rfork(RFENVG) to a init function. That way if you import os, you'll have the Unix semantic of not sharing environment variables between processes. You can still share by using syscall and not importing os. I have a working CL with tests that I'm currently testing.

[gopherbot]

Change https://golang.org/cl/202088 mentions this issue: os: don't share environment variables between processes on Plan 9

[gopherbot]

Change https://golang.org/cl/202657 mentions this issue: syscall: fix Clearenv race on Plan 9

[gopherbot]

Change https://golang.org/cl/236520 mentions this issue: runtime, syscall: use local cache for Setenv/Getenv in Plan 9