Subject: Re: File descriptor leak

From: Sergey Matveev <[email-masked]>
Date: Sun, 8 May 2022 14:42:55 +0300

Greetings!

*** Andrew Chambers [2022-05-08 16:44]:
>I am fairly sure this is a
>file descriptor leak in goredo, as I don't have any problems with other redo
>implementations.

As spacefrogg already answered, goredo opens lock files for all targets
came from the command line. I tried you default.do with musl and saw:

      PID COMM                FD T V FLAGS    REF  OFFSET PRO NAME        
    11263 goredo            text v r r-------   -       - -   /home/stargrave/work/goredo/goredo
    11263 goredo            ctty v c rw------   -       - -   /dev/pts/22       
    11263 goredo             cwd v d r-------   -       - -   /tmp/musl-1.2.3   
    11263 goredo            root v d r-------   -       - -   /                 
    11263 goredo               0 v c r-------   2       0 -   /dev/null         
    11263 goredo               1 p - rw------   4       0 -   -                 
    11263 goredo               2 p - rw------   4       0 -   -                 
    11263 goredo               3 v r -w------   1       0 -   /tmp/musl-1.2.3/src/select/.redo/poll.o.lock
    11263 goredo               4 v r rw------   3       0 -   -                 
    11263 goredo               5 p - rw------  28       0 -   -                 
    11263 goredo               6 p - rw------  31       0 -   -                 
    11263 goredo               7 p - rw------  28       0 -   -                 
    11263 goredo               8 v r -wa-----   3       0 -   /tmp/musl-1.2.3/.redo/.redo.libc.a.rec.8d2431ca
    11263 goredo               9 v r rw------  21   53097 -   -                 
    11263 goredo              10 v r rw------  22       0 -   -                 
    11263 goredo              11 v r -w------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo/timerfd.o.lock
    11263 goredo              12 v r -w------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo/unshare.o.lock
    11263 goredo              13 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.prctl.o.9b587891
    11263 goredo              14 v r -w------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo/utimes.o.lock
    11263 goredo              15 v r -w------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo/vhangup.o.lock
    11263 goredo              16 v r -wa-----   3       0 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.prctl.o.rec.9b55ee9a
    11263 goredo              17 k - rw------   2       0 -   -                 
    11263 goredo              18 p - rw---n--   2       0 -   -                 
    11263 goredo              19 p - rw---n--   1       0 -   -                 
    11263 goredo              20 p - rw---n--   3       0 -   -                 
    11263 goredo              21 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.name_to_handle_at.o.9b4468e0
    11263 goredo              22 v r -wa-----   3       0 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.name_to_handle_at.o.rec.9b41bf26
    11263 goredo              23 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.memfd_create.o.9b341573
    11263 goredo              24 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.pivot_root.o.9b50a895
    11263 goredo              25 v r -wa-----   3   59585 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.memfd_create.o.rec.9b315eb7
    11263 goredo              26 p - rw---n--   3       0 -   -                 
    11263 goredo              27 v r -wa-----   3       0 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.pivot_root.o.rec.9b4e1c31
    11263 goredo              28 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.mount.o.9b404696
    11263 goredo              29 v r -wa-----   3   56373 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.mount.o.rec.9b3dad33
    11263 goredo              30 p - rw---n--   3       0 -   -                 
    11263 goredo              31 p - rw---n--   3       0 -   -                 
    11263 goredo              32 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.personality.o.9b4cb7c2
    11263 goredo              33 v r -wa-----   3       0 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.personality.o.rec.9b4a0ca2
    11263 goredo              34 p - rw---n--   3       0 -   -                 
    11263 goredo              35 p - rw---n--   3       0 -   -                 
    11263 goredo              36 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/signgam.o.lock
    11263 goredo              37 v r rw------   2       0 -   /tmp/musl-1.2.3/src/linux/.redo.ppoll.o.9b5491f0
    11263 goredo              38 v r -wa-----   2       0 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.ppoll.o.rec.9b520a93
    11263 goredo              39 v r rw------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo.gettid.o.9b0f6737
    11263 goredo              40 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/scalblnl.o.lock
    11263 goredo              41 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/significand.o.lock
    11263 goredo              42 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/significandf.o.lock
    11263 goredo              43 v r -w------   1       0 -   /tmp/musl-1.2.3/src/select/.redo/pselect.o.lock
    11263 goredo              44 p - rw---n--   3       0 -   -                 
    11263 goredo              45 p - rw------   7       0 -   -                 
    11263 goredo              46 v r -w------   1       0 -   /tmp/musl-1.2.3/src/locale/.redo/catclose.o.lock
    11263 goredo              47 v c r-------   3       0 -   /dev/null         
    11263 goredo              48 p - rw------   1       0 -   -                 
    11263 goredo              49 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/sinf.o.lock
    11263 goredo              50 v r -wa-----   3   59573 -   /tmp/musl-1.2.3/src/linux/.redo/.redo.gettid.o.rec.9b0cdcb5
    11263 goredo              51 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/sin.o.lock
    11263 goredo              52 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/cbrt.o.lock
    11263 goredo              53 v r -w------   1       0 -   /tmp/musl-1.2.3/src/locale/.redo/catgets.o.lock
    11263 goredo              54 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/sincos.o.lock
    11263 goredo              55 v r -w------   1       0 -   /tmp/musl-1.2.3/src/locale/.redo/c_locale.o.lock
    11263 goredo              56 v r -w------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo/sethostname.o.lock
    11263 goredo              57 v r -w------   1       0 -   /tmp/musl-1.2.3/src/misc/.redo/setdomainname.o.lock
    11263 goredo              58 p - rw---n--   3       0 -   -                 
    11263 goredo              61 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/asinhl.o.lock
    11263 goredo              62 v r -w------   1       0 -   /tmp/musl-1.2.3/src/linux/.redo/wait4.o.lock
    11263 goredo              63 v r -w------   1       0 -   /tmp/musl-1.2.3/src/stdio/.redo/fseek.o.lock
    11263 goredo              64 v r -w------   1       0 -   /tmp/musl-1.2.3/src/mman/.redo/mmap.o.lock
    11263 goredo              65 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/__invtrigl.o.lock
    11263 goredo              66 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/sinh.o.lock
    11263 goredo              67 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/cbrtf.o.lock
    11263 goredo              68 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/atan2f.o.lock
    11263 goredo              69 v r -w------   1       0 -   /tmp/musl-1.2.3/src/math/.redo/cbrtl.o.lock
    [more than thousand of opened lock files]

and it was run with more than a thousand of targets indeed.

spacefrogg advised to use xargs to limit number of arguments passed.
That will work and I like that. Making artificial "xargs"-like limiter
inside goredo leads to the question: how many arguments should be
processed at once? Of course that could be configurable, but that is an
additional burden for the end-user, completely the same as adding xargs.

For example github.com/leahneukirchen/redo-c does not have the problems
with many opened lockfiles, because it takes jobserver's token before it
opens those lockfiles. I explicitly decided to take jobserver's token
after we really decided that target must be done and run. Goredo opens
lockfiles, checks various metainformation, makes some preparations and
temporary files and only after that it acquires jobserver's token to run
the do-file itself. I explicitly wanted to acquire token as late as
possible.

But that leads to possibility of many simultaneously opened (maximally
prepared target before acquiring the jobserver's token and running the
command itself), that, as I can see, brings more complications and
distractions to the end user.

So let's acquire jobserver's token (that already acts as a limiter for
number of jobs simultaneously running) before we open the lockfile. That
will solve the problem of many opened lockfiles. If you pass unlimited
number of jobs (-j 0), then of course it will open as many files as it
wish.

There is a drawback: we can acquire jobserver's token, open the
lockfile, process metadata and see that actually that target was already
done, so we return the jobserver's token back -- it was useless and no
do-file was actually run. But I did not notice any performance
differences in practice, because all of that seems to be anyway pretty
fast enough and not deserving the attention.

So with 1.25.0 that kind of problems should pass away. I do not like
that, but seems that is pretty acceptable compromise, with only a minor
changes in the code.

I also overviewed my code again and found several places where file
descriptors were not closed. But all of them were only related to failed
situations or supplementary helper commands.

By the way, just a remark, your default.do contains "set -o pipefail",
that does not exist in POSIX shell. For example FreeBSD's /bin/sh tells:
    set: Illegal option -o pipefail

-- 
Sergey Matveev (http://www.stargrave.org/)
OpenPGP: CF60 E89A 5923 1E76 E263  6422 AE1A 8109 E498 57EF