Re: The Shepherd on Fibers

[Maxim Cournoyer]

Hi Ludo!

Ludovic Courtès <ludo@gnu.org> writes:

> Hello Guix!
>
> I have pushed a ‘wip-fibers’ branch of the Shepherd:
>
>   https://git.savannah.gnu.org/cgit/shepherd.git/log/?h=wip-fibers
>
> The goal is to make shepherd (the daemon) use Fibers¹ for concurrency.

Very exciting work!  I can't wait to have my 5 min boot reduced (mostly
busy-waiting on the child-hurd VM to start) ;-).

> To illustrate that, it introduces a new, incredible feature: logging!
> As you know, shepherd had (euphemism ahead) limited support for logging,
> essentially in the form of #:log-file, which would redirect a daemon’s
> stdout/stderr to a file, and also in the form of “let’s hope the daemon
> talks to syslogd”.  Here I was able to trivially add logging, such that
> anything spawned by ‘fork+exec-command’ is logged, with timestamps and
> all (the logger is a fiber that calls ‘read-line’ in a loop, which is
> automagically non-blocking—delightful!).  The next fun step (besides
> logging) will be adding inetd-style and/or systemd-style “socket
> activation”.

Interesting; in a (unmerged) patch I had made to mcron, I had extended
the select loop in a way that it'd detect available output and use a
suspendable port to accomplish this; perhaps Fibers would be a
cleaner/more straightforward way to accomplish this.  Also, I was
particularly happy about how flexible the timestamp/metadata prefixed to
each line could be configured by a user; you may want to steal ideas
from it! [0]

[0]  https://lists.gnu.org/archive/html/bug-mcron/2021-08/msg00008.html

> Fibers is used in a single-threaded fashion, which is the main
> constraint for shepherd since it forks.  That also means that fibers
> cannot be preempted, so it’s fully cooperative scheduling.

I see.  I was puzzle by this configure.ac check:

--8<---------------cut here---------------start------------->8---
+dnl Check for extra dependencies.
+GUILE_MODULE_AVAILABLE([have_fibers], [(fibers)])
+if test "x$have_fibers" != "xyes"; then
+  AC_MSG_ERROR([Fibers is missing; please install it.])
+fi
+
+dnl Make sure Fibers does not create POSIX threads: since shepherd
+dnl forks, it must be single-threaded.
+AC_CACHE_CHECK([whether Fibers might create POSIX threads],
+  [ac_cv_fibers_creates_pthreads],
+  [GUILE_CHECK([retval],
+    [(use-modules (fibers))
+     (set! (@ (ice-9 threads) call-with-new-thread)
+       (lambda _ (throw 'new-thread!)))
+     (run-fibers (lambda () (spawn-fiber (lambda () 1)))
+                 #:parallelism 1 #:hz 0)])
+   if test "$retval" = 0; then
+     ac_cv_fibers_creates_pthreads="no"
+   else
+     ac_cv_fibers_creates_pthreads="yes"
+   fi])
+if test "x$ac_cv_fibers_creates_pthreads" = "xyes"; then
+  AC_MSG_ERROR([Fibers creates POSIX threads behind our back; aborting.])
+fi
--8<---------------cut here---------------end--------------->8---

In which scenario would Fibers create unwanted POSIX threads?  Is it
that older versions of Fibers had that problem?  If so, the message
could be more explicit (Fibers too old, you need Fibers >= N).

> There’s one catch: Fibers is currently Linux-only.  The good news is
> that work has been done to port it to other kernels via libevent².
> Until it is merged, we could keep using the Shepherd 0.8 on GNU/Hurd.

Sounds promising.

> I’ve done some Guix System testing in VMs and didn’t notice any major
> issues.  I’d like to merge that branch in ‘master’ and to eventually
> release it as 0.9.0 (with or without socket activation, we’ll see.)
> Hopefully, we could be running it within a couple of weeks.
>
>
> Thoughts?

Nice!

Here's a very naive review (I'm not yet comfortable with the continuation
barriers and others Fibers specifics):

1. In fae59fb * build: Capture the source and object directories of
Fibers.

Why must we even capture the Fibers source/compiled objects directories?
Can't we let Guile and GUILE_LOAD_PATH (or the FHS) handle this for us?
Messing with the load path system dynamically directly is usually a code
smell, in my book.

2. nitpick; in "service: 'read-pid-file' no longer blocks":

--8<---------------cut here---------------start------------->8---
-;; Copyright (C) 2013, 2014, 2015, 2016, 2017, 2018, 2019, 2020, 2021 Ludovic 
Courtès <ludo@gnu.org>
+;; Copyright (C) 2013-2022 Ludovic Courtès <ludo@gnu.org>
--8<---------------cut here---------------end--------------->8---

While I agree this reads better, I've recently stumbled in info
'(maintain) Copyright Notices' on:

      You can use a range (‘2008-2010’) instead of listing individual years
   (‘2008, 2009, 2010’) if and only if: 1) every year in the range,
   inclusive, really is a “copyrightable” year that would be listed
   individually; _and_ 2) you make an explicit statement in a ‘README’ file
   about this usage.

So you'd want to add an explicit statement in the README about it.

3. I was a bit skeptical about the ability to configure the encoding of
the log file; in which situation would switching it to something else
than UTF-8 be useful?

Thanks!

I look forward to see this in mainline.

Maxim