activation: close files after creating listeners #250

vincentbernat · 2018-02-27T15:27:12Z

This is an adaptation of #215 to ensure tests don't fail and extend
the behaviour to PacketConns(). A bit of test coverage is lost, but
just testing if variable environments are not unset in the simple case
doesn't seem worth adding more code.

Fix #215

vincentbernat · 2018-02-27T15:42:35Z

Travis failure seems to be unrelated to this change.

vincentbernat · 2018-02-27T15:59:58Z

However, it should be noted that I don't see any downside to not close the file descriptor. Whatever we do, systemd will keep its own file descriptor, therefore, it's not possible for a user to completely close the associated file description.

lucab · 2018-02-27T16:18:56Z

Existing examples seem to hint at the fact that received FDs could be re-used multiple times, however #215 (comment) seems to state that the original FD is closed/gone after this change.

I need to double-check systemd side for the expected semantics, and golang land for FileListener implementation, so I'll avoid stating something dumb and off here for the moment.

Just for reference, what is the root-cause for this PR? An actual bug/leakage reported somewhere?

vincentbernat · 2018-02-27T16:34:15Z

I was just looking at the open issues and pull requests and thought that closing the file descriptor was a good idea. But as I say in my last comment, I don't see any downside of leaving the file descriptor unclosed because closing it has no effect because systemd is keeping its own open.

lucab · 2018-02-28T15:52:36Z

@vincentbernat I think #252 tackled this transient test failure, can you please rebase both your PRs on current master?

vincentbernat · 2018-02-28T16:33:04Z

Done. But after a day, I think this one could just be closed.

lucab · 2018-02-28T17:05:12Z

Thanks, tests seem greener, but I'm re-triggering travis just to be sure.

I'm leaving this open as a reminder to myself to have a proper look into the File-closing part. I guess the same questions applies to #251, so in the worst case it will help in reviewing that.

lucab · 2018-03-01T15:22:00Z

So, I spent a bit of time looking into this today and these are my comments at the end of the investigation.

net.FileListener and net.FilePacketConn internally perform a dup() on the fd, so in theory we could call Close() on the original File object without issues. However when unsetEnv is false the user may want/try to re-take those fds in future (unlikely, but possible), thus we should not close the external fd in such case.
An additional complication is that stdlib File finalizer unconditionally closes its fd, so reusing fds with unsetEnv = false is inherently racing with the GC.

As such, I would propose to re-purpose this PR to do the following:

call f.Close conditionally only when unsetEnv == true
adjust all docstrings to mention that re-using file-descriptors with unsetEnv = false may race with the GC
remove activation.Listeners(false) (and all similar occurrences) from examples/activation

@vincentbernat how does this sound to you?

vincentbernat · 2018-03-01T15:58:43Z

If there is a risk of the original fd to be closed by the GC after the first File instance, I would be more in favor of closing them and not letting the user reuse them. Otherwise, we expose them to heisenbugs they may or may not source here. Otherwise, we dup them ourselves before putting them in a File.

lucab · 2018-03-01T16:45:04Z

I would be more in favor of closing them and not letting the user reuse them

In which case we probably want to remove the unsetEnv argument too.

Otherwise, we dup them ourselves before putting them in a File.

That would be another option. I'm not a great fan of dup-ing several time the fds we receive, though.
My longer plan was to probe the stdlib folks and see if they would accept an additional method on File to relinquish the fd ownership. That way we don't need to break the API here, and we plug the GC race. As this race always existed and nobody hit it yet so far, I didn't want to block on that right now.

vincentbernat · 2018-03-01T16:48:41Z

If we keep this as is, we already get your proposed behavior since the File instances will get GC at some point (whatever the value of unsetEnv is). We just need to add a note in the documentation (and maybe modify examples like in the PR to not promote reuse?).

lucab · 2018-03-01T18:09:15Z

You are technically correct, sorry if I didn't go into details enough. Even though golang garbage-collects Files (and fds) on its own all the time, as far as I know it is idiomatic to manually close them as soon as it is known that they are not needed anymore (I think primarily to avoid ulimit issues). This was the missing context around my comment on conditionally doing that.

lucab · 2018-03-19T13:39:37Z

activation/listeners.go

@@ -32,6 +32,7 @@ func Listeners(unsetEnv bool) ([]net.Listener, error) {
 	for i, f := range files {
 		if pc, err := net.FileListener(f); err == nil {
 			listeners[i] = pc
+			f.Close()


This would better be:

if unsetEnv { f.Close() }

lucab · 2018-03-19T13:40:34Z

activation/packetconns.go

@@ -31,6 +31,7 @@ func PacketConns(unsetEnv bool) ([]net.PacketConn, error) {
 	for i, f := range files {
 		if pc, err := net.FilePacketConn(f); err == nil {
 			conns[i] = pc
+			f.Close()


Same as above, this would be conditional on unsetEnv.

lucab · 2018-03-19T13:44:41Z

Ping. I've opened a discussion on Go File semantics at golang/go#24215, but so far it has not gone too far. As such, I've left a few comments on this PR for the things we can fix now. For later, we can re-consider whether to drop the unsetEnv=false case at all, but there is no need to block this PR on that.

This is an adaptation of coreos#215 to ensure tests don't fail and extend the behaviour to PacketConns(). A bit of test coverage is lost, but just testing if variable environments are not unset in the simple case doesn't seem worth adding more code. Fix coreos#215

vincentbernat · 2018-03-19T14:09:56Z

OK, PR updated. I have restored the tests as they work as is now.

lucab

LGTM

lucab · 2018-03-20T11:29:24Z

So, the File.Relinquish() proposal got turned down. While it can be somehow worked around here, from a maintainability point of view I'd like to avoid re-implementing half of the stdlib, or having three duplicate sets of fds.

I'm more inclined to just remove the unsetEnv options from all Listeners*() (forcing it to always unset), and only keep it where we can transfer File ownership back to the caller (i.e. in Files()).
@vincentbernat what do you think?

vincentbernat · 2018-03-20T13:13:29Z

I agree with you.

lucab self-requested a review February 27, 2018 16:19

lucab added needs/review needs/more-information labels Feb 27, 2018

lucab mentioned this pull request Feb 28, 2018

test: make dbus subscription tests more leniant #252

Merged

vincentbernat force-pushed the fix/close-listener branch from b2904e2 to 41c34bd Compare February 28, 2018 16:32

lucab mentioned this pull request Mar 7, 2018

activation: close files after creating listeners #215

Closed

lucab suggested changes Mar 19, 2018

View reviewed changes

lucab added needs/rework and removed needs/more-information needs/review labels Mar 19, 2018

lucab mentioned this pull request Mar 19, 2018

activation: add support for listeners with names #251

Merged

vincentbernat force-pushed the fix/close-listener branch from 41c34bd to 4e7a0c4 Compare March 19, 2018 14:09

lucab approved these changes Mar 19, 2018

View reviewed changes

lucab removed the needs/rework label Mar 19, 2018

lucab merged commit 4f97ab3 into coreos:master Mar 19, 2018

lucab added the kind/bugfix label May 7, 2018

lucab added this to the v17 milestone May 7, 2018

lucab mentioned this pull request May 7, 2018

activation: remove unsafe support for fd re-use #276

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

activation: close files after creating listeners #250

activation: close files after creating listeners #250

vincentbernat commented Feb 27, 2018

vincentbernat commented Feb 27, 2018

vincentbernat commented Feb 27, 2018

lucab commented Feb 27, 2018 •

edited

vincentbernat commented Feb 27, 2018

lucab commented Feb 28, 2018

vincentbernat commented Feb 28, 2018

lucab commented Feb 28, 2018 •

edited

lucab commented Mar 1, 2018

vincentbernat commented Mar 1, 2018

lucab commented Mar 1, 2018

vincentbernat commented Mar 1, 2018

lucab commented Mar 1, 2018 •

edited

lucab Mar 19, 2018

lucab Mar 19, 2018

lucab commented Mar 19, 2018 •

edited

vincentbernat commented Mar 19, 2018

lucab left a comment

lucab commented Mar 20, 2018

vincentbernat commented Mar 20, 2018

activation: close files after creating listeners #250

activation: close files after creating listeners #250

Conversation

vincentbernat commented Feb 27, 2018

vincentbernat commented Feb 27, 2018

vincentbernat commented Feb 27, 2018

lucab commented Feb 27, 2018 • edited

vincentbernat commented Feb 27, 2018

lucab commented Feb 28, 2018

vincentbernat commented Feb 28, 2018

lucab commented Feb 28, 2018 • edited

lucab commented Mar 1, 2018

vincentbernat commented Mar 1, 2018

lucab commented Mar 1, 2018

vincentbernat commented Mar 1, 2018

lucab commented Mar 1, 2018 • edited

lucab Mar 19, 2018

Choose a reason for hiding this comment

lucab Mar 19, 2018

Choose a reason for hiding this comment

lucab commented Mar 19, 2018 • edited

vincentbernat commented Mar 19, 2018

lucab left a comment

Choose a reason for hiding this comment

lucab commented Mar 20, 2018

vincentbernat commented Mar 20, 2018

lucab commented Feb 27, 2018 •

edited

lucab commented Feb 28, 2018 •

edited

lucab commented Mar 1, 2018 •

edited

lucab commented Mar 19, 2018 •

edited