bug#34407: Shepherd won't close socket on exit

bug#34407: Shepherd won't close socket on exit

[nly]

Shepherd does not close the socket when exiting with `herd stop root`.
"bind: Address already in use"

~$ rm /run/user/1000/shepherd/socket
~$ shepherd
Service root has been started.
~$ herd status
error: connect: /run/user/1000/shepherd/socket: Connection refused
~$ shepherd
Service root has been started.
~$ Backtrace:
           3 (primitive-load "/run/current-system/profile/bin/shepherd")
In shepherd.scm:
   250:24  2 (main . _)
     48:6  1 (open-server-socket _)
In unknown file:
           0 (bind #<input-output: socket 13> #(1 "/run/user/1000/shepherd/socket") #)

ERROR: In procedure bind:
In procedure bind: Address already in use
  C-c C-c
~$ 

bug#34407: Acknowledgement (Shepherd won't close socket on exit)

[nly]

Looks like I pasted something wrong in the previous message. I realized
after I saw it on the mail.

This time I've checked it twice. Left the shepherd in a weird limbo
where connection is refused to the old socket and cannot create new
connection. 

Ofcourse, i can `rm /run/user/1000/shepherd/socket`
--------------------------------------------------------------------------------
nly@uf ~$ herd status
error: connect: /run/user/1000/shepherd/socket: No such file or directory
nly@uf ~$ shepherd
Service root has been started.
nly@uf ~$ herd status
Started:
 + root
Stopped:
 - icecat
 - jack
 - mpv
 - mpv-jack
 - tor
 - transmission
nly@uf ~$ herd stop root
nly@uf ~$ herd status
error: connect: /run/user/1000/shepherd/socket: Connection refused
nly@uf ~$ shepherd
Service root has been started.
nly@uf ~$ Backtrace:
           3 (primitive-load "/run/current-system/profile/bin/shepherd")
In shepherd.scm:
   250:24  2 (main . _)
     48:6  1 (open-server-socket _)
In unknown file:
           0 (bind #<input-output: socket 13> #(1 "/run/user/1000/shepherd/socket") #)

ERROR: In procedure bind:
In procedure bind: Address already in use
  C-c C-c
nly@uf ~$ herd status
error: connect: /run/user/1000/shepherd/socket: Connection refused

bug#34407: Acknowledgement (Shepherd won't close socket on exit)

[Maxim Cournoyer]

Hi,

nly <nly@disroot.org> writes:

> Looks like I pasted something wrong in the previous message. I realized
> after I saw it on the mail.
>
> This time I've checked it twice. Left the shepherd in a weird limbo
> where connection is refused to the old socket and cannot create new
> connection. 
>
> Ofcourse, i can `rm /run/user/1000/shepherd/socket`
> --------------------------------------------------------------------------------
> nly@uf ~$ herd status
> error: connect: /run/user/1000/shepherd/socket: No such file or directory
> nly@uf ~$ shepherd
> Service root has been started.
> nly@uf ~$ herd status
> Started:
>  + root
> Stopped:
>  - icecat
>  - jack
>  - mpv
>  - mpv-jack
>  - tor
>  - transmission
> nly@uf ~$ herd stop root
> nly@uf ~$ herd status
> error: connect: /run/user/1000/shepherd/socket: Connection refused
> nly@uf ~$ shepherd
> Service root has been started.
> nly@uf ~$ Backtrace:
>            3 (primitive-load "/run/current-system/profile/bin/shepherd")
> In shepherd.scm:
>    250:24  2 (main . _)
>      48:6  1 (open-server-socket _)
> In unknown file:
>            0 (bind #<input-output: socket 13> #(1 "/run/user/1000/shepherd/socket") #)
>
> ERROR: In procedure bind:
> In procedure bind: Address already in use
>   C-c C-c
> nly@uf ~$ herd status
> error: connect: /run/user/1000/shepherd/socket: Connection refused

This has been annoying me as well; my current workaround is to put this
in my ~/.xsession:

--8<---------------cut here---------------start------------->8---
# Start user services
rm -f /run/user/1000/shepherd/socket
shepherd
--8<---------------cut here---------------end--------------->8---

Maxim

[PATCH] shepherd: Delete the socket file upon exit.

[宋文武]

[-- Attachment #1: Type: text/plain, Size: 211 bytes --]

Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
script too...

According to 'man 2 bind', the socket pathname should be deleted when no
longer required, so a patch to fix this bug:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-shepherd-Delete-the-socket-file-upon-exit.patch --]
[-- Type: text/x-patch, Size: 3675 bytes --]

From f171f6adb2fc6ee3bf4d25378c2e7bba109b43d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzsong@member.fsf.org>
Date: Sun, 17 Feb 2019 11:27:28 +0800
Subject: [PATCH] shepherd: Delete the socket file upon exit.

Fixes <https://bugs.gnu.org/34407>.

* modules/shepherd.scm (call-with-server-socket): New procedure.
(main): Use it instead of 'open-server-socket'.
---
 modules/shepherd.scm | 65 ++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index e241e7a..314b989 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -49,6 +49,17 @@
       (listen sock 10)
       sock)))
 
+(define (call-with-server-socket file-name proc)
+  "Call PROC, passing it a listening socket at FILE-NAME and deleting the
+socket file at FILE-NAME upon exit of PROC.  Return the values of PROC."
+  (let ((sock (open-server-socket file-name)))
+    (dynamic-wind
+      noop
+      (lambda () (proc sock))
+      (lambda ()
+        (close sock)
+        (delete-file file-name)))))
+
 \f
 ;; Main program.
 (define (main . args)
@@ -256,32 +267,34 @@
           ;; Get commands from the standard input port.
           (process-textual-commands (current-input-port))
           ;; Process the data arriving at a socket.
-          (let ((sock   (open-server-socket socket-file)))
-
-            ;; Possibly write out our PID, which means we're ready to accept
-            ;; connections.  XXX: What if we daemonized already?
-            (match pid-file
-              ((? string? file)
-               (with-atomic-file-output pid-file
-                 (cute display (getpid) <>)))
-              (#t (display (getpid)))
-              (_  #t))
-
-            (let next-command ()
-              (define (read-from sock)
-                (match (accept sock)
-                  ((command-source . client-address)
-                   (setvbuf command-source (buffering-mode block) 1024)
-                   (process-connection command-source))
-                  (_ #f)))
-              (match (select (list sock) (list) (list) (if poll-services? 0.5 #f))
-                (((sock) _ _)
-                 (read-from sock))
-                (_
-                 #f))
-              (when poll-services?
-                (check-for-dead-services))
-              (next-command)))))))
+          (call-with-server-socket
+           socket-file
+           (lambda (sock)
+
+             ;; Possibly write out our PID, which means we're ready to accept
+             ;; connections.  XXX: What if we daemonized already?
+             (match pid-file
+               ((? string? file)
+                (with-atomic-file-output pid-file
+                  (cute display (getpid) <>)))
+               (#t (display (getpid)))
+               (_  #t))
+
+             (let next-command ()
+               (define (read-from sock)
+                 (match (accept sock)
+                   ((command-source . client-address)
+                    (setvbuf command-source (buffering-mode block) 1024)
+                    (process-connection command-source))
+                   (_ #f)))
+               (match (select (list sock) (list) (list) (if poll-services? 0.5 #f))
+                 (((sock) _ _)
+                  (read-from sock))
+                 (_
+                  #f))
+               (when poll-services?
+                 (check-for-dead-services))
+               (next-command))))))))
 
 ;; Start all of SERVICES, which is a list of canonical names (FIXME?),
 ;; but in a order where all dependencies are fulfilled before we
-- 
2.19.2

Re: [PATCH] shepherd: Delete the socket file upon exit.

[Danny Milosavljevic]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Sun, 17 Feb 2019 11:38:16 +0800
iyzsong@member.fsf.org (宋文武) wrote:

> Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
> script too...
> 
> According to 'man 2 bind', the socket pathname should be deleted when no
> longer required, so a patch to fix this bug:

Hmm, I guess you can do that.

But /run is supposed to be a tmpfs and elogind is supposed to rm -rf /run/user/1000
after all sessions of that user terminated in any case, so how is it left over
in the first place?

If the deletion in the case above doesn't work, please report a bug.

If that patch is only in order to enable users to restart user's shepherd
without exiting all their sessions, then I guess that's ok--although unusual.

Does your patch do the right thing if the user's shepherd is already
running? (i.e. keep the socket file)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] shepherd: Delete the socket file upon exit.

[宋文武]

Danny Milosavljevic <dannym@scratchpost.org> writes:

> On Sun, 17 Feb 2019 11:38:16 +0800
> iyzsong@member.fsf.org (宋文武) wrote:
>
>> Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
>> script too...
>> 
>> According to 'man 2 bind', the socket pathname should be deleted when no
>> longer required, so a patch to fix this bug:
>
> Hmm, I guess you can do that.
>
> But /run is supposed to be a tmpfs and elogind is supposed to rm -rf /run/user/1000
> after all sessions of that user terminated in any case, so how is it left over
> in the first place?
>

Well, maybe the elogind version I used didn't have this feature, or I
had another user session running...

> If the deletion in the case above doesn't work, please report a bug.

Thanks, good to know, and it indeed works.

>
> If that patch is only in order to enable users to restart user's shepherd
> without exiting all their sessions, then I guess that's ok--although unusual.
>
> Does your patch do the right thing if the user's shepherd is already
> running? (i.e. keep the socket file)

Yes, it deletes the socket file at exit (not at startup).

bug#34407: [PATCH] shepherd: Delete the socket file upon exit.

[宋文武]

Danny Milosavljevic <dannym@scratchpost.org> writes:

> On Sun, 17 Feb 2019 11:38:16 +0800
> iyzsong@member.fsf.org (宋文武) wrote:
>
>> Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
>> script too...
>> 
>> According to 'man 2 bind', the socket pathname should be deleted when no
>> longer required, so a patch to fix this bug:
>
> Hmm, I guess you can do that.
>
> But /run is supposed to be a tmpfs and elogind is supposed to rm -rf /run/user/1000
> after all sessions of that user terminated in any case, so how is it left over
> in the first place?
>

Well, maybe the elogind version I used didn't have this feature, or I
had another user session running...

> If the deletion in the case above doesn't work, please report a bug.

Thanks, good to know, and it indeed works.

>
> If that patch is only in order to enable users to restart user's shepherd
> without exiting all their sessions, then I guess that's ok--although unusual.
>
> Does your patch do the right thing if the user's shepherd is already
> running? (i.e. keep the socket file)

Yes, it deletes the socket file at exit (not at startup).

bug#34407: [PATCH] shepherd: Delete the socket file upon exit.

[Danny Milosavljevic]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

On Sun, 17 Feb 2019 11:38:16 +0800
iyzsong@member.fsf.org (宋文武) wrote:

> Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
> script too...
> 
> According to 'man 2 bind', the socket pathname should be deleted when no
> longer required, so a patch to fix this bug:

Hmm, I guess you can do that.

But /run is supposed to be a tmpfs and elogind is supposed to rm -rf /run/user/1000
after all sessions of that user terminated in any case, so how is it left over
in the first place?

If the deletion in the case above doesn't work, please report a bug.

If that patch is only in order to enable users to restart user's shepherd
without exiting all their sessions, then I guess that's ok--although unusual.

Does your patch do the right thing if the user's shepherd is already
running? (i.e. keep the socket file)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

bug#34407: [PATCH] shepherd: Delete the socket file upon exit.

[Ludovic Courtès]

Hello,

iyzsong@member.fsf.org (宋文武) skribis:

> Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
> script too...

I never had to do that because /run is wiped at boot time, like Danny
wrote.

> According to 'man 2 bind', the socket pathname should be deleted when no
> longer required, so a patch to fix this bug:
>
> From f171f6adb2fc6ee3bf4d25378c2e7bba109b43d8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzsong@member.fsf.org>
> Date: Sun, 17 Feb 2019 11:27:28 +0800
> Subject: [PATCH] shepherd: Delete the socket file upon exit.
>
> Fixes <https://bugs.gnu.org/34407>.
>
> * modules/shepherd.scm (call-with-server-socket): New procedure.
> (main): Use it instead of 'open-server-socket'.

Pushed, thanks!

Ludo’.

Re: bug#34407: [PATCH] shepherd: Delete the socket file upon exit.

[Ludovic Courtès]

Hello,

iyzsong@member.fsf.org (宋文武) skribis:

> Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
> script too...

I never had to do that because /run is wiped at boot time, like Danny
wrote.

> According to 'man 2 bind', the socket pathname should be deleted when no
> longer required, so a patch to fix this bug:
>
> From f171f6adb2fc6ee3bf4d25378c2e7bba109b43d8 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzsong@member.fsf.org>
> Date: Sun, 17 Feb 2019 11:27:28 +0800
> Subject: [PATCH] shepherd: Delete the socket file upon exit.
>
> Fixes <https://bugs.gnu.org/34407>.
>
> * modules/shepherd.scm (call-with-server-socket): New procedure.
> (main): Use it instead of 'open-server-socket'.

Pushed, thanks!

Ludo’.

bug#34407: [PATCH] shepherd: Delete the socket file upon exit.

[宋文武]

[-- Attachment #1: Type: text/plain, Size: 211 bytes --]

Yes, I have the 'rm /run/user/1000/shepherd/socket' workaround in my session
script too...

According to 'man 2 bind', the socket pathname should be deleted when no
longer required, so a patch to fix this bug:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-shepherd-Delete-the-socket-file-upon-exit.patch --]
[-- Type: text/x-patch, Size: 3675 bytes --]

From f171f6adb2fc6ee3bf4d25378c2e7bba109b43d8 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E5=AE=8B=E6=96=87=E6=AD=A6?= <iyzsong@member.fsf.org>
Date: Sun, 17 Feb 2019 11:27:28 +0800
Subject: [PATCH] shepherd: Delete the socket file upon exit.

Fixes <https://bugs.gnu.org/34407>.

* modules/shepherd.scm (call-with-server-socket): New procedure.
(main): Use it instead of 'open-server-socket'.
---
 modules/shepherd.scm | 65 ++++++++++++++++++++++++++------------------
 1 file changed, 39 insertions(+), 26 deletions(-)

diff --git a/modules/shepherd.scm b/modules/shepherd.scm
index e241e7a..314b989 100644
--- a/modules/shepherd.scm
+++ b/modules/shepherd.scm
@@ -49,6 +49,17 @@
       (listen sock 10)
       sock)))
 
+(define (call-with-server-socket file-name proc)
+  "Call PROC, passing it a listening socket at FILE-NAME and deleting the
+socket file at FILE-NAME upon exit of PROC.  Return the values of PROC."
+  (let ((sock (open-server-socket file-name)))
+    (dynamic-wind
+      noop
+      (lambda () (proc sock))
+      (lambda ()
+        (close sock)
+        (delete-file file-name)))))
+
 \f
 ;; Main program.
 (define (main . args)
@@ -256,32 +267,34 @@
           ;; Get commands from the standard input port.
           (process-textual-commands (current-input-port))
           ;; Process the data arriving at a socket.
-          (let ((sock   (open-server-socket socket-file)))
-
-            ;; Possibly write out our PID, which means we're ready to accept
-            ;; connections.  XXX: What if we daemonized already?
-            (match pid-file
-              ((? string? file)
-               (with-atomic-file-output pid-file
-                 (cute display (getpid) <>)))
-              (#t (display (getpid)))
-              (_  #t))
-
-            (let next-command ()
-              (define (read-from sock)
-                (match (accept sock)
-                  ((command-source . client-address)
-                   (setvbuf command-source (buffering-mode block) 1024)
-                   (process-connection command-source))
-                  (_ #f)))
-              (match (select (list sock) (list) (list) (if poll-services? 0.5 #f))
-                (((sock) _ _)
-                 (read-from sock))
-                (_
-                 #f))
-              (when poll-services?
-                (check-for-dead-services))
-              (next-command)))))))
+          (call-with-server-socket
+           socket-file
+           (lambda (sock)
+
+             ;; Possibly write out our PID, which means we're ready to accept
+             ;; connections.  XXX: What if we daemonized already?
+             (match pid-file
+               ((? string? file)
+                (with-atomic-file-output pid-file
+                  (cute display (getpid) <>)))
+               (#t (display (getpid)))
+               (_  #t))
+
+             (let next-command ()
+               (define (read-from sock)
+                 (match (accept sock)
+                   ((command-source . client-address)
+                    (setvbuf command-source (buffering-mode block) 1024)
+                    (process-connection command-source))
+                   (_ #f)))
+               (match (select (list sock) (list) (list) (if poll-services? 0.5 #f))
+                 (((sock) _ _)
+                  (read-from sock))
+                 (_
+                  #f))
+               (when poll-services?
+                 (check-for-dead-services))
+               (next-command))))))))
 
 ;; Start all of SERVICES, which is a list of canonical names (FIXME?),
 ;; but in a order where all dependencies are fulfilled before we
-- 
2.19.2