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

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Christopher Baines
Owner
unassigned
Submitted by
Christopher Baines
Severity
normal
C
C
Christopher Baines wrote on 17 Nov 2023 19:05
(address . guix-patches@gnu.org)
460cdfa67b473ea2f1593668b2d9d0fd159378d0.1700244314.git.mail@cbaines.net
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(-)

Toggle diff (29 lines)
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
L
L
Ludovic Courtès wrote on 26 Nov 2023 23:16
(name . Christopher Baines)(address . mail@cbaines.net)
87fs0s17op.fsf@gnu.org
Hi Christopher,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (12 lines)
> 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!)

Toggle quote (10 lines)
> +++ 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:

Toggle snippet (25 lines)
(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 ...)))

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’.
C
C
Christopher Baines wrote on 27 Nov 2023 10:48
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 67245@debbugs.gnu.org)
875y1nmrwf.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (21 lines)
> 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
:)

Toggle quote (13 lines)
>> +++ 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?
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmVkakBfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XcoJhAArMwV2vXM3qOOR01PaYfTHmQb/NcB3XiK
VXiusYS9WB15qLYF8v3qxhXtvhy/4ZBmmTCe9l/adwjqGqoHAeBqSRHNODvCZG3+
KXEclNOe1/7zWFlIkzP3bjHBQr9qOHdEq2NLl+Btyzn/9c8WGYeR0SWCgY4rjDtX
fUicq8czkuAURryhErg1wyDXgHdNwt4mez7sH7Tx3biBSC71PRmc/Cjbn5n3bhM9
cqJ8BWpel198VsDAJpCF+s6PvmfHRMCvuFie0IVl6O3wuvGkMoO6VofMg8i0mlsE
sBh97Cio+ShZRhtqjYP6L50cNnQNwH3WT7QfhG0qMOPJJCje21MRckmyfWiot4pp
BQmYVmyBxzOesKsfWvexuCeQN7BF9XAkFgf+vu2xfCmjxTNdCrqB/6YswVqa4/Z2
oIB0hiWEMhQxC0bCDU2dIExi1LkrXlAWQJPxPYxwaz3VnT3fUy7zHW5Ql2iXFd/r
2Sr8Iy7MvGVpmwyaQA3LgLcDji/h1vxQpzqniZDFaqFPMgdSEcxCG3wKGzZIsPAJ
MtyA5FPYxPHt042bWrGMNdyiVMpBpSZvzfgexCpJi96jikfD9tkWOyEjxgUnk7Jo
4QA03acFYUPxFkIiey6mJOFvzVzeTogFMqe6sV7/Bvz2fWv/dSczx8MbWNaEeoRP
qEmCKTgUzvE=
=iswm
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 30 Nov 2023 22:11
(name . Christopher Baines)(address . mail@cbaines.net)(address . 67245@debbugs.gnu.org)
87fs0nndy9.fsf@gnu.org
Hi Chris,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (9 lines)
> 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. :-)

Toggle quote (21 lines)
>>> +++ 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’.
C
C
Christopher Baines wrote on 11 May 18:53 +0200
[PATCH v2] store: Add with-store/non-blocking.
(address . 67245@debbugs.gnu.org)
e692ef2de80732b3ed87e95a489f053616f4130a.1715446400.git.mail@cbaines.net
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(-)

Toggle diff (146 lines)
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
C
C
Christopher Baines wrote on 12 May 19:38 +0200
Re: [bug#67245] [PATCH] store: Use a non-blocking socket for store connections.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 67245@debbugs.gnu.org)
87bk5b7wwu.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (36 lines)
>>>> +++ 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.
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmZA/qFfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9Xfg6xAAs1SQRJtgIZUhwaIey4Fv1Moc4A0j8Rto
0QPVUBa3bqiXeAxyFkXTxSW4ke8jILjzhTcnmDk16lijmP+FPYN57JIVu0lLQDQf
PkggqF8eJ0Te8born4s4v+5O601xKVH3zeZJl3BETIETVC+F4Rd7SbpmDxlAq10C
PUaaoPRGphTdbLmgXc6srN7+tmSQFt4SiDMmv9vKNs1Wll47slEBRdacq7V8Gc+8
b5cdYXpOTloGhVFMf6ZJLnlm2kViy9LFKHKTciWej5xgutzwjUht2oQxGUeoYHV7
huFVHNAizYjM4k0uoGF8qi9jutev0PTvIWZ5b3GXih6iFtoZcFbyuqN3siqxYlnJ
1rKat5VYhqED8cHgg+73ywz+1n6B5gBXT+2tgdI98uAZ1vaD2M2f7taSIfHtjMio
qQ035phm1MrGu3F8udtVoO4YCCHEq3Dj8MxSRd9m+q0c6KYwerkq2c5i8JFyGuT2
OPU5xSyjrn+A6ZWzxKHMIyisTD7X2tYmHE2W8cd8fzt7Fr4rimcHaLq69obmhhrb
V9cZOqL4Ohtgd03BwLipq+GLEzSCFkbEFgqPJkDY06uXr3ZvPP2q+wLuTLQWj7IE
mShUN8qKTjOVgZfwU6jKJofghAIBWUeBNRhYXKxsWsf0kUxWBkKd67shQPluyalI
vwbaCfLqfKA=
=/9YT
-----END PGP SIGNATURE-----

L
L
Ludovic Courtès wrote on 13 May 14:44 +0200
Re: [bug#67245] [PATCH v2] store: Add with-store/non-blocking.
(name . Christopher Baines)(address . mail@cbaines.net)
87seylankt.fsf@gnu.org
Hi,

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (20 lines)
> 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

[...]

Toggle quote (12 lines)
> +(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.

Toggle quote (5 lines)
> +(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’.
C
C
Christopher Baines wrote on 13 May 21:32 +0200
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 67245-done@debbugs.gnu.org)
878r0d33un.fsf@cbaines.net
Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (40 lines)
> 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.

Toggle quote (8 lines)
>> +(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.

Toggle quote (2 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iQKlBAEBCgCPFiEEPonu50WOcg2XVOCyXiijOwuE9XcFAmZCasBfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcRHG1haWxAY2Jh
aW5lcy5uZXQACgkQXiijOwuE9XdVjxAAnKw2ImrI99fhS9eJGeSIjqF3/gudibar
vJ4DqtDCIYpsvfOMTlDpO/+HLRRAZSZDh5YNQfQmcW+T+KiEMCsglTZBI3j3f40V
DZh7MLu4hannLtuFnnu78Ck3lrE7ro7tv7t8VoG5Wfp5ceSmjHJ/LUqdkPt4R22k
/ztJURBepuS7ENoyt5+jX/5C/cLjnUQYXsl6W6Z5qihOhwpqLSaFGbmj8OqNGJ4L
ULUoLjnRsDplNCuRf1afFYMHHhNNlWj3y1CdrtUwHEIxuTtptclknv/MSl/JLsCf
2qQsXQ5o71mJZ5SIOU9wKfUvBD9SBqumsZD1xLnZhYikngqzjttxY8gA6ca4FaVh
Klk/mXukC4RRZt9CBcYS+whiURCVzueB+PNUR3ptVZ9aFQ2ZeiP3AWQ8U/3ubg6C
6RuiRvldo7eBWXQRlfA/vix7SdnaG5KZfTBTqzQnBNpQl4z6lPDCcNr3vQcM8p96
eohG784Hn1QJo8XqWo0PkSkoRTGmCifaHFSsunUGuAw/Vm2tAwV21ku+8xF1lJ/M
zyRELFz1RB4CK+4YCOf4sJ0JJcC7curHuwnY+XCJIxctZ4n65VJzia+T6TbxgHmN
LnszLl8+GFLkssIIWSmTOraaHmc9JR269V+v8IVkgkD/zTrHqPFKTCVMzBSh09RK
yx5g3Gr+l0M=
=41fg
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 67245@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 67245
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch