activation: remove unsafe support for fd re-use #276
Conversation
|
/cc @squeed @vincentbernat for review This is a followup to #250 (comment). It contains a breaking change for the |
|
I also had to bump the ubuntu image we use for tests, as 16.04 comes with an ancient go compiler, and as one of our dependencies (godbus) started using |
|
OK for me. |
|
|
|
@vincentbernat yes, we only use it that way. But it is part of the public API and its usage is actually safe, so I plan to keep it as is. |
|
Looks fine to me! Do you want to add something to the docstring like "you should pass unsetEnv as true unless you know what you're doing"? |
Package `activation` was previously offering an option to leave environment variables untouched, in order to re-use fd at a future point. However, re-using fd is unsafe and results in racing against the GC due to the way how `os.File` is implemented. Moreover, the Go runtime does not (nor plan to) provide a safe interface for proper temporary borrowing of existing fd. Thus, this removes the `unsetEnv` option from all listeners (forcing to always unset), and only keeps it where ownership can be transferred back to the caller.
lucab
force-pushed
the
ups/listeners-always-unset
branch
from
8a2b307 to
0c07474
Compare
|
@squeed good idea. I added a couple of lines to describe flag rationale and typical usage. Everything else kept untouched, PTAL. |
Package
activationwas previously offering an option to leaveenvironment variables untouched, in order to re-use fd at a future
point.
However, re-using fd is unsafe and results in racing against the GC
due to the way how
os.Fileis implemented. Moreover, the Go runtimedoes not (nor plan to) provide a safe interface for proper temporary
borrowing of existing fd.
Thus, this removes the
unsetEnvoption from all listeners (forcingto always unset), and only keeps it where ownership can be transferred
back to the caller.
Ref: #250 (comment)
Ref: golang/go#24215