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

add function to return all inherited files #68

Merged
merged 1 commit into from Mar 21, 2022

Conversation

hunts
Copy link
Contributor

@hunts hunts commented Mar 17, 2022

This is helpful in the use case of integrating with systemd socket activation.

When there is no parent, the app need to scan or use actication.Files() to
find all the fds and call upgrader.AddFile() to track them. But when there is a
parent, with the help of this new function, we can find the fds from upgrader
instance rather then by scanning fds or calling actication.Files() both of which
creates a set of os.File representing the underlying fds.

Without this change, there would be two set of os.File objects that represent
the same fd set. One set of os.File is hold by the upgrader while the another set
returning from fd scanning (or actication.Files()) is being used at other places.
This raise a risk of closing the same fd twice at different time which is really bad
when the fd number get reused by other files during the two close calls.

This is helpful in the use case of integrating with systemd socket activation.

When there is no parent, the app still need to scan or use actication.Files() to
find all the fds and call upgrader.AddFile() to track them. But when there is a
parent, we can access the fds from upgrader instance rather then by scanning
fds or calling actication.Files() both of which creates os.File objects for the fds.

Without this change, there would be two set of os.File objects that represent
the same fd set. One set of os.File is hold by the upgrader whie the another set
returning from fd scanning (or actication.Files()) is being used at other places.
This raise a risk of closing the same fd twice at different time which is really bad
when the fd get reused by other files during the two close calls.
@lmb
Copy link
Contributor

lmb commented Mar 21, 2022

Hi Hunts,

If I remember correctly, files are dup()ed before being added to Fds:

tableflip/fds.go

Lines 316 to 319 in 287e7a4

file, err := dupConn(conn, key)
if err != nil {
return fmt.Errorf("can't dup %s (%s %s): %s", kind, network, addr, err)
}

This means that you can close the file you passed to Upgrader.Fds.AddFile after the call returns without creating a problem. In pseudo code, I'd probably do something like this:

upg, err := ...

if !upg.HasParent() {
  for _, f := range systemdInerited {
    err := upg.AddFile // upg.AddListener, etc.
    f.Close()  
  }
}

// Continue normal start up
ln, err := upg.Listen("tcp", ...)

Does that solve your problem?

@lmb
Copy link
Contributor

lmb commented Mar 21, 2022

(NB: I don't have merge access to this repository anymore!)

@hunts
Copy link
Contributor Author

hunts commented Mar 21, 2022

The problem is occurring at this step:

// Continue normal start up
ln, err := upg.Listen("tcp", ...)

We don't have named addresses but a list of fd passed from systemd. If you still remember this piece of code:

/etc/systemd/system/<service-name>.socket:
  file.managed:
{%- for addr in ["127.0.0.1", "[::1]"] %}
  {%- for _ in range(0, 128) %}
          - ListenDatagram: "{{ addr }}:53"
  {%- endfor %}
{%- endfor %}

What we can do is either like:

// Continue normal start up
for _, f := range systemdInerited {
    ln, err := net.FileConn(f) 
    ...
}

Or like:

// Continue normal start up
files, err := upg.Files()
for _, f := range files {
    ln, err := net.FileConn(f) 
    ...
}

The former approach has the problem we I've describe in the PR message. The later one requires the Files() methods we are introducing here.

@jdesgats jdesgats merged commit cd67dfb into cloudflare:master Mar 21, 2022
@hunts hunts deleted the hunts/access-all-inherited-files branch March 21, 2022 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants