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

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

Open
millerresearch opened this issue May 3, 2018 · 11 comments
Open
Milestone

Comments

@millerresearch
Copy link

@millerresearch millerresearch commented May 3, 2018

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 bradfitz added the OS-Plan9 label May 3, 2018
@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 3, 2018

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

@bradfitz bradfitz added this to the Unplanned milestone May 3, 2018
@bradfitz bradfitz added the NeedsFix label May 3, 2018
@cherrymui

This comment has been minimized.

Copy link
Contributor

@cherrymui cherrymui commented May 3, 2018

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

@millerresearch

This comment has been minimized.

Copy link
Author

@millerresearch millerresearch commented May 3, 2018

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

This comment has been minimized.

Copy link
Contributor

@rminnich rminnich commented May 4, 2018

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 4, 2018

@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

This comment has been minimized.

Copy link
Author

@millerresearch millerresearch commented May 4, 2018

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 4, 2018

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

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 18, 2019

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.

@bcmills bcmills changed the title syscall: sharing of environment variables in Plan 9 doesn't match go expectations os: sharing of environment variables in Plan 9 doesn't match go expectations Oct 18, 2019
@fhs

This comment has been minimized.

Copy link
Contributor

@fhs fhs commented Oct 19, 2019

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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 19, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 22, 2019

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

gopherbot pushed a commit that referenced this issue Oct 23, 2019
Update #25234
Fixes #35083

Change-Id: Ida39516ab1c14a34a62c2232476a75e83f4e3f75
Reviewed-on: https://go-review.googlesource.com/c/go/+/202657
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants
You can’t perform that action at this time.