[bug#67245] [PATCH] store: Use a non-blocking socket for store connections.

[bug#67245] [PATCH] store: Use a non-blocking socket for store connections.

[Christopher Baines]

For some applications, it's important to do this here rather than just making
the socket non-blocking after the connection is established because there can
be I/O on the socket that will block during the handshake.

I've noticed this blocking during the handshake causing issues in the build
coordinator for example.

* guix/store.scm (open-unix-domain-socket, open-inet-socket): Pass
SOCK_NONBLOCK when calling socket.

Change-Id: I8225762b78448bc1f7b698c8de5d736e13f577bf
---
 guix/store.scm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/guix/store.scm b/guix/store.scm
index f8e77b2cd9..216be98c05 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -460,7 +460,9 @@ (define (open-unix-domain-socket file)
 '&store-connection-error' upon error."
   (let ((s (with-fluids ((%default-port-encoding #f))
              ;; This trick allows use of the `scm_c_read' optimization.
-             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
+             (socket PF_UNIX
+                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
+                     0)))
         (a (make-socket-address PF_UNIX file)))
 
     (system-error-to-connection-error file
@@ -488,7 +490,8 @@ (define (open-inet-socket host port)
       ((ai rest ...)
        (let ((s (socket (addrinfo:fam ai)
                         ;; TCP/IP only
-                        (logior SOCK_STREAM SOCK_CLOEXEC) IPPROTO_IP)))
+                        (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
+                        IPPROTO_IP)))
 
          (catch 'system-error
            (lambda ()

base-commit: e35b7c5386c1bfacf47ed31bac9b503373dd26fc
-- 
2.41.0

[bug#67245] [PATCH] store: Use a non-blocking socket for store connections.

[Ludovic Courtès]

Hi Christopher,

Christopher Baines <mail@cbaines.net> skribis:

> For some applications, it's important to do this here rather than just making
> the socket non-blocking after the connection is established because there can
> be I/O on the socket that will block during the handshake.
>
> I've noticed this blocking during the handshake causing issues in the build
> coordinator for example.
>
> * guix/store.scm (open-unix-domain-socket, open-inet-socket): Pass
> SOCK_NONBLOCK when calling socket.
>
> Change-Id: I8225762b78448bc1f7b698c8de5d736e13f577bf

I feel we should really discuss on Guix + Fibers since we’ve apparently
been going through the exact same set of issues.  :-)

(The other thing that comes to mind is the resource pool!)

> +++ b/guix/store.scm
> @@ -460,7 +460,9 @@ (define (open-unix-domain-socket file)
>  '&store-connection-error' upon error."
>    (let ((s (with-fluids ((%default-port-encoding #f))
>               ;; This trick allows use of the `scm_c_read' optimization.
> -             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
> +             (socket PF_UNIX
> +                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
> +                     0)))

We cannot do this here because callers have to be prepared to deal with
non-blocking sockets, and that’s not the case in Guix itself.

In Cuirass, I have this:

--8<---------------cut here---------------start------------->8---
(define (non-blocking-port port)
  "Make PORT non-blocking and return it."
  (let ((flags (fcntl port F_GETFL)))
    (when (zero? (logand O_NONBLOCK flags))
      (fcntl port F_SETFL (logior O_NONBLOCK flags)))
    port))

(define (ensure-non-blocking-store-connection store)
  "Mark the file descriptor that backs STORE, a <store-connection>, as
O_NONBLOCK."
  (match (store-connection-socket store)
    ((? file-port? port)
     (non-blocking-port port))
    (_ #f)))

(define-syntax-rule (with-store/non-blocking store exp ...)
  "Like 'with-store', bind STORE to a connection to the store, but ensure that
said connection is non-blocking (O_NONBLOCK).  Evaluate EXP... in that
context."
  (with-store store
    (ensure-non-blocking-store-connection store)
    (let ()
      exp ...)))
--8<---------------cut here---------------end--------------->8---

Then ‘with-store/non-blocking’ is used in fiberized context where I know
this is fine.

I think it’ll have to remain this way until Guix itself is fiberized or
something.

Does that make sense?

Ludo’.

[bug#67245] [PATCH] store: Use a non-blocking socket for store connections.

[Christopher Baines]

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


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

> Hi Christopher,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> For some applications, it's important to do this here rather than just making
>> the socket non-blocking after the connection is established because there can
>> be I/O on the socket that will block during the handshake.
>>
>> I've noticed this blocking during the handshake causing issues in the build
>> coordinator for example.
>>
>> * guix/store.scm (open-unix-domain-socket, open-inet-socket): Pass
>> SOCK_NONBLOCK when calling socket.
>>
>> Change-Id: I8225762b78448bc1f7b698c8de5d736e13f577bf
>
> I feel we should really discuss on Guix + Fibers since we’ve apparently
> been going through the exact same set of issues.  :-)
>
> (The other thing that comes to mind is the resource pool!)

I'm mostly ignoring these issues then coping the code once you write it
:)

>> +++ b/guix/store.scm
>> @@ -460,7 +460,9 @@ (define (open-unix-domain-socket file)
>>  '&store-connection-error' upon error."
>>    (let ((s (with-fluids ((%default-port-encoding #f))
>>               ;; This trick allows use of the `scm_c_read' optimization.
>> -             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
>> +             (socket PF_UNIX
>> +                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
>> +                     0)))
>
> We cannot do this here because callers have to be prepared to deal with
> non-blocking sockets, and that’s not the case in Guix itself.

I can see potential problems for programs outside of Guix which use
suspendable ports, but given Guix doesn't use suspendable ports, this
won't change behaviour, right?

Obviously Guile will be working a bit differently, using poll when it
needs to wait for I/O, but at the scheme level within Guix, things
should be no different.

I tried guix weather with this change, and things seemed fine. Is there
a specific bit of Guix you're concerned about?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#67245] [PATCH] store: Use a non-blocking socket for store connections.

[Ludovic Courtès]

Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>> I feel we should really discuss on Guix + Fibers since we’ve apparently
>> been going through the exact same set of issues.  :-)
>>
>> (The other thing that comes to mind is the resource pool!)
>
> I'm mostly ignoring these issues then coping the code once you write it
> :)

Heh, so we’re already in sync maybe, not bad.  :-)

>>> +++ b/guix/store.scm
>>> @@ -460,7 +460,9 @@ (define (open-unix-domain-socket file)
>>>  '&store-connection-error' upon error."
>>>    (let ((s (with-fluids ((%default-port-encoding #f))
>>>               ;; This trick allows use of the `scm_c_read' optimization.
>>> -             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
>>> +             (socket PF_UNIX
>>> +                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
>>> +                     0)))
>>
>> We cannot do this here because callers have to be prepared to deal with
>> non-blocking sockets, and that’s not the case in Guix itself.
>
> I can see potential problems for programs outside of Guix which use
> suspendable ports, but given Guix doesn't use suspendable ports, this
> won't change behaviour, right?
>
> Obviously Guile will be working a bit differently, using poll when it
> needs to wait for I/O, but at the scheme level within Guix, things
> should be no different.

Hmm yes, I think you’re right.

One issue is if we hand over the file descriptor to something that’s not
Guile.  Off the top of my head, this happens with inferiors and in the
‘build’ procedure of ‘build-self.scm’ (well, the process that receives
that file descriptor is Guile, but if it’s older than 3.0 (?), then it
may behave differently.)

So it should be safe indeed, but adds a bit of overhead (hopping via
‘current-{read,write}-waiter’) and needs good testing.

Laziness gives an incentive for the status quo, but I’m not opposed to
the change if we get more confidence (test suite passing, tests with
inferiors and ‘time-machine’, and some more auditing.)

Ludo’.

[bug#67245] [PATCH v2] store: Add with-store/non-blocking.

[Christopher Baines]

For some applications, it's important to establish a non-blocking connection
rather than just making the socket non-blocking after the connection is
established. This is because there is I/O on the socket that will block during
the handshake.

I've noticed this blocking during the handshake causing issues in the build
coordinator for example.

This commit adds a new with-store variant to avoid changing the behaviour of
with-store/open-connection to ensure that this change can't break anything
that depends on the blocking nature of the socket.

* guix/store.scm (open-unix-domain-socket, open-inet-socket): Take
 #:non-blocking? and use SOCK_NONBLOCK when calling socket if appropriate.
(connect-to-daemon, open-connection, call-with-store): Take #:non-blocking?
and pass it on.
(with-store/non-blocking): New syntax rule.

Change-Id: I8225762b78448bc1f7b698c8de5d736e13f577bf
---
 guix/store.scm | 53 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 37 insertions(+), 16 deletions(-)

diff --git a/guix/store.scm b/guix/store.scm
index a238cb627a..3e8202a43a 100644
--- a/guix/store.scm
+++ b/guix/store.scm
@@ -106,6 +106,7 @@ (define-module (guix store)
             port->connection
             close-connection
             with-store
+            with-store/non-blocking
             set-build-options
             set-build-options*
             valid-path?
@@ -462,12 +463,15 @@ (define-syntax-rule (system-error-to-connection-error file exp ...)
                            (file file)
                            (errno errno))))))))
 
-(define (open-unix-domain-socket file)
+(define* (open-unix-domain-socket file #:key non-blocking?)
   "Connect to the Unix-domain socket at FILE and return it.  Raise a
-'&store-connection-error' upon error."
+'&store-connection-error' upon error.  If NON-BLOCKING?, make the socket
+non-blocking."
   (let ((s (with-fluids ((%default-port-encoding #f))
              ;; This trick allows use of the `scm_c_read' optimization.
-             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
+             (socket PF_UNIX
+                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
+                     0)))
         (a (make-socket-address PF_UNIX file)))
 
     (system-error-to-connection-error file
@@ -478,9 +482,10 @@ (define %default-guix-port
   ;; Default port when connecting to a daemon over TCP/IP.
   44146)
 
-(define (open-inet-socket host port)
+(define* (open-inet-socket host port #:key non-blocking?)
   "Connect to the Unix-domain socket at HOST:PORT and return it.  Raise a
-'&store-connection-error' upon error."
+'&store-connection-error' upon error.  If NON-BLOCKING?, make the socket
+non-blocking."
   (define addresses
     (getaddrinfo host
                  (if (number? port) (number->string port) port)
@@ -495,7 +500,10 @@ (define (open-inet-socket host port)
       ((ai rest ...)
        (let ((s (socket (addrinfo:fam ai)
                         ;; TCP/IP only
-                        (logior SOCK_STREAM SOCK_CLOEXEC) IPPROTO_IP)))
+                        (if non-blocking?
+                            (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
+                            (logior SOCK_STREAM SOCK_CLOEXEC))
+                        IPPROTO_IP)))
 
          (catch 'system-error
            (lambda ()
@@ -514,9 +522,10 @@ (define (open-inet-socket host port)
                                     (errno (system-error-errno args)))))
                  (loop rest)))))))))
 
-(define (connect-to-daemon uri)
+(define* (connect-to-daemon uri #:key non-blocking?)
   "Connect to the daemon at URI, a string that may be an actual URI or a file
-name, and return an input/output port.
+name, and return an input/output port.  If NON-BLOCKING?, use a non-blocking
+socket when using the file, unix or guix URI schemes.
 
 This is a low-level procedure that does not perform the initial handshake with
 the daemon.  Use 'open-connection' for that."
@@ -533,11 +542,13 @@ (define (connect-to-daemon uri)
        (match (uri-scheme uri)
          ((or #f 'file 'unix)
           (lambda (_)
-            (open-unix-domain-socket (uri-path uri))))
+            (open-unix-domain-socket (uri-path uri)
+                                     #:non-blocking? non-blocking?)))
          ('guix
           (lambda (_)
             (open-inet-socket (uri-host uri)
-                              (or (uri-port uri) %default-guix-port))))
+                              (or (uri-port uri) %default-guix-port)
+                              #:non-blocking? non-blocking?)))
          ((? symbol? scheme)
           ;; Try to dynamically load a module for SCHEME.
           ;; XXX: Errors are swallowed.
@@ -557,7 +568,8 @@ (define (connect-to-daemon uri)
   (connect uri))
 
 (define* (open-connection #:optional (uri (%daemon-socket-uri))
-                          #:key port (reserve-space? #t) cpu-affinity)
+                          #:key port (reserve-space? #t) cpu-affinity
+                          non-blocking?)
   "Connect to the daemon at URI (a string), or, if PORT is not #f, use it as
 the I/O port over which to communicate to a build daemon.
 
@@ -565,7 +577,9 @@ (define* (open-connection #:optional (uri (%daemon-socket-uri))
 space on the file system so that the garbage collector can still operate,
 should the disk become full.  When CPU-AFFINITY is true, it must be an integer
 corresponding to an OS-level CPU number to which the daemon's worker process
-for this connection will be pinned.  Return a server object."
+for this connection will be pinned.  If NON-BLOCKING?, use a non-blocking
+socket when using the file, unix or guix URI schemes.  Return a server
+object."
   (define (handshake-error)
     (raise (condition
             (&store-connection-error (file (or port uri))
@@ -577,7 +591,8 @@ (define* (open-connection #:optional (uri (%daemon-socket-uri))
              ;; really a connection error.
              (handshake-error)))
     (let*-values (((port)
-                   (or port (connect-to-daemon uri)))
+                   (or port (connect-to-daemon
+                             uri #:non-blocking? non-blocking?)))
                   ((output flush)
                    (buffering-output-port port
                                           (make-bytevector 8192))))
@@ -657,9 +672,10 @@ (define (close-connection server)
   "Close the connection to SERVER."
   (close (store-connection-socket server)))
 
-(define (call-with-store proc)
-  "Call PROC with an open store connection."
-  (let ((store (open-connection)))
+(define* (call-with-store proc #:key non-blocking?)
+  "Call PROC with an open store connection.  Pass NON-BLOCKING? to
+open-connection."
+  (let ((store (open-connection #:non-blocking? non-blocking?)))
     (define (thunk)
       (parameterize ((current-store-protocol-version
                       (store-connection-version store)))
@@ -678,6 +694,11 @@ (define-syntax-rule (with-store store exp ...)
 automatically close the store when the dynamic extent of EXP is left."
   (call-with-store (lambda (store) exp ...)))
 
+(define-syntax-rule (with-store/non-blocking store exp ...)
+  "Bind STORE to an non-blocking open connection to the store and evaluate
+EXPs; automatically close the store when the dynamic extent of EXP is left."
+  (call-with-store (lambda (store) exp ...) #:non-blocking? #t))
+
 (define current-store-protocol-version
   ;; Protocol version of the store currently used.  XXX: This is a hack to
   ;; communicate the protocol version to the build output port.  It's a hack

base-commit: 9288654773a110156e0bb6fc703a9c24f5bfc527
-- 
2.41.0

[bug#67245] [PATCH] store: Use a non-blocking socket for store connections.

[Christopher Baines]

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

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

>>>> +++ b/guix/store.scm
>>>> @@ -460,7 +460,9 @@ (define (open-unix-domain-socket file)
>>>>  '&store-connection-error' upon error."
>>>>    (let ((s (with-fluids ((%default-port-encoding #f))
>>>>               ;; This trick allows use of the `scm_c_read' optimization.
>>>> -             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
>>>> +             (socket PF_UNIX
>>>> +                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
>>>> +                     0)))
>>>
>>> We cannot do this here because callers have to be prepared to deal with
>>> non-blocking sockets, and that’s not the case in Guix itself.
>>
>> I can see potential problems for programs outside of Guix which use
>> suspendable ports, but given Guix doesn't use suspendable ports, this
>> won't change behaviour, right?
>>
>> Obviously Guile will be working a bit differently, using poll when it
>> needs to wait for I/O, but at the scheme level within Guix, things
>> should be no different.
>
> Hmm yes, I think you’re right.
>
> One issue is if we hand over the file descriptor to something that’s not
> Guile.  Off the top of my head, this happens with inferiors and in the
> ‘build’ procedure of ‘build-self.scm’ (well, the process that receives
> that file descriptor is Guile, but if it’s older than 3.0 (?), then it
> may behave differently.)
>
> So it should be safe indeed, but adds a bit of overhead (hopping via
> ‘current-{read,write}-waiter’) and needs good testing.
>
> Laziness gives an incentive for the status quo, but I’m not opposed to
> the change if we get more confidence (test suite passing, tests with
> inferiors and ‘time-machine’, and some more auditing.)

Maybe we can just move the with-store/non-blocking in to Guix, as that
will solve the immediate issue.

I've sent a new patch for that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]

[bug#67245] [PATCH v2] store: Add with-store/non-blocking.

[Ludovic Courtès]

Hi,

Christopher Baines <mail@cbaines.net> skribis:

> For some applications, it's important to establish a non-blocking connection
> rather than just making the socket non-blocking after the connection is
> established. This is because there is I/O on the socket that will block during
> the handshake.
>
> I've noticed this blocking during the handshake causing issues in the build
> coordinator for example.
>
> This commit adds a new with-store variant to avoid changing the behaviour of
> with-store/open-connection to ensure that this change can't break anything
> that depends on the blocking nature of the socket.
>
> * guix/store.scm (open-unix-domain-socket, open-inet-socket): Take
>  #:non-blocking? and use SOCK_NONBLOCK when calling socket if appropriate.
> (connect-to-daemon, open-connection, call-with-store): Take #:non-blocking?
> and pass it on.
> (with-store/non-blocking): New syntax rule.
>
> Change-Id: I8225762b78448bc1f7b698c8de5d736e13f577bf

[...]

> +(define* (open-unix-domain-socket file #:key non-blocking?)
>    "Connect to the Unix-domain socket at FILE and return it.  Raise a
> -'&store-connection-error' upon error."
> +'&store-connection-error' upon error.  If NON-BLOCKING?, make the socket
> +non-blocking."
>    (let ((s (with-fluids ((%default-port-encoding #f))
>               ;; This trick allows use of the `scm_c_read' optimization.
> -             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
> +             (socket PF_UNIX
> +                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
> +                     0)))

Make sure SOCK_NONBLOCK is added only when ‘non-blocking?’ is true.

> +(define-syntax-rule (with-store/non-blocking store exp ...)
> +  "Bind STORE to an non-blocking open connection to the store and evaluate
> +EXPs; automatically close the store when the dynamic extent of EXP is left."
> +  (call-with-store (lambda (store) exp ...) #:non-blocking? #t))

I think we’ll need an entry in ‘.dir-locals.el’ and one in (guix
read-print) for proper formatting.

OK for me with these changes!

Thanks,
Ludo’.

bug#67245: [PATCH v2] store: Add with-store/non-blocking.

[Christopher Baines]

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

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

> Hi,
>
> Christopher Baines <mail@cbaines.net> skribis:
>
>> For some applications, it's important to establish a non-blocking connection
>> rather than just making the socket non-blocking after the connection is
>> established. This is because there is I/O on the socket that will block during
>> the handshake.
>>
>> I've noticed this blocking during the handshake causing issues in the build
>> coordinator for example.
>>
>> This commit adds a new with-store variant to avoid changing the behaviour of
>> with-store/open-connection to ensure that this change can't break anything
>> that depends on the blocking nature of the socket.
>>
>> * guix/store.scm (open-unix-domain-socket, open-inet-socket): Take
>>  #:non-blocking? and use SOCK_NONBLOCK when calling socket if appropriate.
>> (connect-to-daemon, open-connection, call-with-store): Take #:non-blocking?
>> and pass it on.
>> (with-store/non-blocking): New syntax rule.
>>
>> Change-Id: I8225762b78448bc1f7b698c8de5d736e13f577bf
>
> [...]
>
>> +(define* (open-unix-domain-socket file #:key non-blocking?)
>>    "Connect to the Unix-domain socket at FILE and return it.  Raise a
>> -'&store-connection-error' upon error."
>> +'&store-connection-error' upon error.  If NON-BLOCKING?, make the socket
>> +non-blocking."
>>    (let ((s (with-fluids ((%default-port-encoding #f))
>>               ;; This trick allows use of the `scm_c_read' optimization.
>> -             (socket PF_UNIX (logior SOCK_STREAM SOCK_CLOEXEC) 0)))
>> +             (socket PF_UNIX
>> +                     (logior SOCK_STREAM SOCK_CLOEXEC SOCK_NONBLOCK)
>> +                     0)))
>
> Make sure SOCK_NONBLOCK is added only when ‘non-blocking?’ is true.

Ah, yep, I've fixed this now.

>> +(define-syntax-rule (with-store/non-blocking store exp ...)
>> +  "Bind STORE to an non-blocking open connection to the store and evaluate
>> +EXPs; automatically close the store when the dynamic extent of EXP is left."
>> +  (call-with-store (lambda (store) exp ...) #:non-blocking? #t))
>
> I think we’ll need an entry in ‘.dir-locals.el’ and one in (guix
> read-print) for proper formatting.

I've added an entry in to .dir-locals.el, but unless I've missed
something, (guix read-print) doesn't handle with-store so maybe we need
to add with-store and with-store/non-blocking.

> OK for me with these changes!

Great, I've pushed to 3db1a8341c815af3673c367518fbb193f5592864 and I can
follow up with the (guix read-print) changes and updating the guix
package later.

Thanks,

Chris

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 987 bytes --]