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

[GNU bug Tracking System]

Your message dated Mon, 13 May 2024 20:32:16 +0100
with message-id <878r0d33un.fsf@cbaines.net>
and subject line Re: [bug#67245] [PATCH v2] store: Add with-store/non-blocking.
has caused the debbugs.gnu.org bug report #67245,
regarding [PATCH] store: Use a non-blocking socket for store connections.
to be marked as done.

(If you believe you have received this mail in error, please contact
help-debbugs@gnu.org.)


-- 
67245: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=67245
GNU Bug Tracking System
Contact help-debbugs@gnu.org with problems
> --- Begin Message ---
>
>
>
> Subject: 
>
> [PATCH] store: Use a non-blocking socket for store connections.
>
>
>
>
> Date: 
>
> Fri, 17 Nov 2023 18:05:14 +0000
>
>
>
>
> 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
>
>
>
>
> --- End Message ---

--- Begin Message --- Subject: Re: [bug#67245] [PATCH v2] store: Add with-store/non-blocking. Date: Mon, 13 May 2024 20:32:16 +0100 User-agent: mu4e 1.12.2; emacs 29.3

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

signature.asc
Description: PGP signature

--- End Message ---