Re: bug#47867: [1.2.1 pre-release testing] substitute downloading and TLS errors

[Ludovic Courtès]

Hi Florian,

(Cc: Chris who’s also familiar with (guix http-client).)

"pelzflorian (Florian Pelz)" <pelzflorian@pelzflorian.de> skribis:

> It still gets stuck (sometimes with enlightenment, one time with
> udisks, restarting the install fixed it once).  After getting stuck,
> this different error message is shown now; no TLS error (copied by
> manual typing, there may be typos):
>
> gtk-doc-1.28  653KiB                2.4MiB/s 00:00 [####################] 
> 100.0%
> udisks-2.8.4  842KiB                1.6MiB/s 00:00 [####################] 
> 100.0%
>
> substitute: updating substitutes from 'https://ci.guix.gnu.org'... 
> 100.0%Backtrace:
> substitute: In ice-9/boot-9.scm:
> substitute:   1736:10 17 (with-exception-handler _ _ #:unwind? _ # _)
> substitute: In unknown file:
> substitute:           16 (apply-smob/0 #<thunk 7fc2dbf60520>)
> substitute: In ice-9/boot-9.scm:
> substitute:     718:2 15 (call-with-prompt _ _ #<procedure 
> default-prompt-handle…>)
> substitute: In ice-9/eval.scm:
> substitute:     619:8 14 (_ #(#(#<directory (guile-user) 7fc2dbf63c80>)))
> substitute: In guix/ui.scm:
> substitute:   2164:12 13 (run-guix-command _ . _)
> substitute: In ice-9/boot-9.scm:
> substitute:   1736:10 12 (with-exception-handler _ _ #:unwind? _ # _)
> substitute:   1736:10 11 (with-exception-handler _ _ #:unwind? _ # _)
> substitute:   1731:15 10 (with-exception-handler _ _ #:unwind? _ # _)
> substitute: In guix/scripts/substitute.scm:
> substitute:    745:18  9 (_)
> substitute:    346:26  8 (process-query #<output: file 4> _ #:cache-urls _ 
> #:acl _)
> substitute: In guix/substitutes.scm:
> substitute:    358:27  7 (lookup-narinfos/diverse _ _ #<procedure 
> 7fc2d3b15000 …>
> substitute:    315:31  6 (lookup-narinfos _ _ #:open-connection _ # _)
> substitute:    238:26  5 (fetch-narinfos _ _ #:open-connection _ # _)
> substitute: In ice-9/boot-9.scm:
> substitute:   1669:16  4 (raise-exception _ #:continuable? _)
> substitute:   1669:16  3 (raise-exception _ #:continuable? _)
> substitute:   1764:13  2 (_ #<&compound-exception _ components: 
> assertion-fail…>)
> substitute:   1669:16  1 (raise-exception _ #:continuable? _)
> substitute:   1669:16  0 (raise-exception _ #:continuable? _)
> substitute:
> substitute: In ice-9/boot-9.scm:1669:16 In procedure raise-exception:
> substitute: In procedure %read-line: Wrong type argument in position 1 
> (expecting open input port): #<closed: string 7fc2d8796070>
> guix system: error: 
> `/gnu/store/k3n98i1fk9awd5ydv4ry4k4rlpp7i13m7-guix-1.2.0-22.c467718/bin/guix 
> substitute' died unexpectedly

I think I got it: commit 205833b72c5517915a47a50dbe28e7024dc74e57 (then
carried over in 45fce38fb0b6c6796906149ade145b8d3594c1c6) introduced a
call to ‘connect’ in non-tail position.  Once that recursive call to
‘connect’ had completed, ‘http-multiple-get’ would go on in ‘loop’
trying to re-process responses, but at that point there aren’t any
responses left to process.

This problem could only happen if a networking exception would occur
while sending HTTP requests for narinfos.  Thus, it was most likely to
occur when interleaving substitutions and queries, as in the snippet you
provided above, because then ‘http-multiple-get’ was more likely to be
passed a stale reused connection.

Florian, could you try again with the attached patch?

If you have the courage, it would be awesome if you could also try the
patch without the ‘error/again’ bits.  It’s possible that they aren’t
needed now.  I double-checked and the GnuTLS Guile bindings already
handle GNUTLS_E_AGAIN and GNUTLS_E_INTERRUPTED, so my guess is that this
was just a side effect of dealing with stale TLS sessions:

  https://gitlab.com/gnutls/gnutls/-/blob/master/guile/src/core.c#L1042

Thanks a lot for your help!

Ludo’.

diff --git a/guix/http-client.scm b/guix/http-client.scm
index a2e11a1b73..b9cf0b1a4b 100644
--- a/guix/http-client.scm
+++ b/guix/http-client.scm
@@ -38,7 +38,7 @@
   #:use-module (guix utils)
   #:use-module (guix base64)
   #:autoload   (gcrypt hash) (sha256)
-  #:autoload   (gnutls) (error/invalid-session)
+  #:autoload   (gnutls) (error/invalid-session error/again)
   #:use-module ((guix build utils)
                 #:select (mkdir-p dump-port))
   #:use-module ((guix build download)
@@ -163,7 +163,8 @@ reusing stale cached connections."
       (if (or (and (eq? key 'system-error)
                    (= EPIPE (system-error-errno `(,key ,@args))))
               (and (eq? key 'gnutls-error)
-                   (eq? (first args) error/invalid-session))
+                   (memq (first args)
+                         (list error/again error/invalid-session)))
               (memq key
                     '(bad-response bad-header bad-header-component)))
           #f
@@ -207,15 +208,14 @@ returning."
         ;; Inherit the HTTP proxying property from P.
         (set-http-proxy-port?! buffer (http-proxy-port? p))
 
-        (unless (false-if-networking-error
-                 (begin
-                   (for-each (cut write-request <> buffer) batch)
-                   (put-bytevector p (get))
-                   (force-output p)
-                   #t))
-          ;; If PORT becomes unusable, open a fresh connection and retry.
-          (close-port p)                          ; close the broken port
-          (connect #f requests result)))
+        ;; Swallow networking errors that could occur due to connection reuse
+        ;; and the like; they will be handled down the road when trying to
+        ;; read responses.
+        (false-if-networking-error
+         (begin
+           (for-each (cut write-request <> buffer) batch)
+           (put-bytevector p (get))
+           (force-output p))))
 
       ;; Now start processing responses.
       (let loop ((sent      batch)
diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm
index 48309f9b3a..65940591a9 100755
--- a/guix/scripts/substitute.scm
+++ b/guix/scripts/substitute.scm
@@ -45,7 +45,7 @@
                 #:select (uri-abbreviation nar-uri-abbreviation
                           (open-connection-for-uri
                            . guix:open-connection-for-uri)))
-  #:autoload   (gnutls) (error/invalid-session)
+  #:autoload   (gnutls) (error/invalid-session error/again)
   #:use-module (guix progress)
   #:use-module ((guix build syscalls)
                 #:select (set-thread-name))
@@ -417,7 +417,8 @@ server certificates."
         (if (or (and (eq? key 'system-error)
                      (= EPIPE (system-error-errno `(,key ,@args))))
                 (and (eq? key 'gnutls-error)
-                     (eq? (first args) error/invalid-session))
+                     (memq (first args)
+                           (list error/again error/invalid-session)))
                 (memq key '(bad-response bad-header bad-header-component)))
             (proc (open-connection-for-uri/cached uri
                                                   #:verify-certificate? #f