Ludovic Courtès <l...@gnu.org> wrote:
>
> Andrew Whatson <what...@gmail.com> skribis:
>
> > We also need equivalent functionality around SOCK_CLOEXEC. It seems
> > this is implemented for ‘accept’, but not ‘socket’ or ‘socketpair’.
>
> It’s possible to use SOCK_CLOEXEC with ‘socket’ and ‘socketpair’
> already, as in:
>
> (socket AF_INET (logior SOCK_CLOEXEC SOCK_STREAM) 0)
>
> With commit 1d313bf5f0d296d766bd3a0e6d030df37c71711b, ‘pipe’ is also
> covered.
>
> So I think we have pretty much everything we need, at least starting
> with 3.0.9.
Ah, nice! In that case it might be possible to deprecate this
auto-closing behaviour in a future version.
> > Python's PEP 433 contains a good explanation of the issues which can
> > arise from leaked file descriptors:
> > https://peps.python.org/pep-0433/#inherited-file-descriptors-issues
> >
> > Given the risks, I'm convinced that Guile's conservative approach is
> > actually quite sensible. It seems like the best path forward would be
> > to implement platform-specific optimizations where possible.
> >
> > I've attached a draft patch which implements a fast-path on systems
> > where "/proc/self/fd" is available.
>
> The patch LGTM; it’s certainly an improvement on systems configured with
> a high per-process FD limit.
>
> Now, I believe use of ‘posix_spawn’ as proposed in
> <https://issues.guix.gnu.org/52835> makes that unnecessary. Let’s take
> a closer look at that other patch and so we can see…
Playing with the wip-posix-spawn branch, it has the same slowdown
(actually a bit worse). I've updated the "/proc/self/fd" fast-path
patch for posix_spawn, please find attached.
> Thanks,
> Ludo’.
Thank you!
commit c012d7b0d5248a99a3a92780687a676c5d420f5f
Author: Andrew Whatson <what...@gmail.com>
Date: Thu Dec 8 21:43:28 2022 +1000
Reduce redundant close() calls when forking on some systems.
Some systems provide "/proc/self/fd" which is a directory containing an
entry for each open file descriptor in the current process. We use this
to limit the number of close() calls needed to ensure file descriptors
aren't leaked to the child process when forking.
* libguile/posix.c (close_inherited_fds_slow):
(close_inherited_fds): New static helper functions.
(scm_spawn_process): Attempt to close inherited file descriptors
efficiently using 'close_inherited_fds', falling back to the brute-force
approach in 'close_inherited_fds_slow'.
diff --git a/libguile/posix.c b/libguile/posix.c
index 87b329da9..8c9022116 100644
--- a/libguile/posix.c
+++ b/libguile/posix.c
@@ -24,6 +24,7 @@
# include <config.h>
#endif
+#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
@@ -1309,6 +1310,41 @@ SCM_DEFINE (scm_fork, "primitive-fork", 0, 0, 0,
#undef FUNC_NAME
#endif /* HAVE_FORK */
+static void
+close_inherited_fds_slow (posix_spawn_file_actions_t *actions, int max_fd)
+{
+ while (--max_fd > 2)
+ posix_spawn_file_actions_addclose (actions, max_fd);
+}
+
+static void
+close_inherited_fds (posix_spawn_file_actions_t *actions, int max_fd)
+{
+ DIR *dirp;
+ struct dirent *d;
+ int fd;
+
+ /* Try to use the platform-specific list of open file descriptors, so
+ we don't need to use the brute force approach. */
+ dirp = opendir ("/proc/self/fd");
+
+ if (dirp == NULL)
+ return close_inherited_fds_slow (actions, max_fd);
+
+ while ((d = readdir (dirp)) != NULL)
+ {
+ fd = atoi (d->d_name);
+
+ /* Skip "." and "..", garbage entries, stdin/stdout/stderr. */
+ if (fd <= 2)
+ continue;
+
+ posix_spawn_file_actions_addclose (actions, fd);
+ }
+
+ closedir (dirp);
+}
+
static SCM
scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
#define FUNC_NAME "spawn*"
@@ -1363,8 +1399,7 @@ scm_spawn_process (SCM prog, SCM args, SCM scm_in, SCM scm_out, SCM scm_err)
posix_spawn_file_actions_adddup2 (&actions, fd_slot[1], 1);
posix_spawn_file_actions_adddup2 (&actions, fd_slot[2], 2);
- while (--max_fd > 2)
- posix_spawn_file_actions_addclose (&actions, max_fd);
+ close_inherited_fds (&actions, max_fd);
if (posix_spawnp (&pid, exec_file, &actions, attrp, exec_argv, environ) != 0)
{
