Subject: Re: bug#31878: Module autoloading is not thread safe

From: [email-masked] (Ludovic Courtès)
Date: Mon, 22 Oct 2018 12:10:15 +0200

Hi Mark,

Mark H Weaver <[email-masked]> skribis:

> I've written a preliminary patch to implement the improved thread-safe
> module autoloading that I outlined in earlier messages in this bug
> report.

Great, thanks a lot!

On a cursory look, it LGTM.  I wonder if we could somehow split
‘resolve-module’ and ‘try-module-autoload’ into smaller chunks because
they’ve become quite large and complex.

For example:

> +                (lambda ()
> +                  (when mutex
> +                    ((ice-9-threads 'lock-mutex) mutex))
> +                  (set-autoloaded! dir-hint name didit)))
> +
> +              ;; If the local module was actually created, then we
> +              ;; now add it to the global module table.
> +              (let ((module (car lazy-module-cell)))
> +                (when module
> +                  (module-define-submodule! parent-module
> +                                            (car reverse-name)
> +                                            module)))
> +
> +              ;; Signal all threads waiting on the condition variable
> +              ;; for this module to be loaded.
> +              (when cond-var
> +                ((ice-9-threads 'broadcast-condition-variable) cond-var))
> +
> +              ;; Remove the module from '%modules-being-loaded'.
> +              (set! %modules-being-loaded
> +                    (assoc-remove! %modules-being-loaded
> +                                   module-name))
> +
> +              ;; Remove all '%modules-waiting-for' entries that are
> +              ;; directly related to the module that we just loaded
> +              ;; (or attempted to load).
> +              (set! %modules-waiting-for
> +                    (filter! (lambda (entry)
> +                               (not (or (equal? module-name (car entry))
> +                                        (equal? module-name (cdr entry)))))
> +                             %modules-waiting-for))

Perhaps this bit could go in a ‘record-module-autoload-completion!’
procedure or something along these lines?

It’s great that you came up with tests to reproduce the problems.  I
confirm that “./check-guile threads.test” works for me.  The tests pass
even if I remove the {boot-9,threads}.scm changes, though.

However peg.test fails and it may be related:

--8<---------------cut here---------------start------------->8---
$ ./check-guile peg.test
Testing /data/src/guile-2.1/meta/guile ... peg.test
with GUILE_LOAD_PATH=/data/src/guile-2.1/test-suite
Running peg.test
Backtrace:
In ice-9/eval.scm:
   293:34 19 (_ #<directory (guile-user) 1f5c140>)
In ice-9/boot-9.scm:
   3032:4 18 (define-module* _ #:filename _ #:pure _ #:version _ #:imports _ #:exports _ #:replacements _ # _ # _ \u2026)
  2071:24 17 (call-with-deferred-observers #<procedure 1f06eb0 at ice-9/boot-9.scm:3033:5 ()>)
  3045:24 16 (_)
   222:29 15 (map1 (((test-suite lib)) ((ice-9 peg)) ((ice-9 pretty-print)) ((srfi srfi-1))))
   222:17 14 (map1 (((ice-9 peg)) ((ice-9 pretty-print)) ((srfi srfi-1))))
  2958:17 13 (resolve-interface (ice-9 peg) #:select _ #:hide _ #:prefix _ #:renamer _ #:version _)
In ice-9/threads.scm:
    390:8 12 (_ _)
In ice-9/boot-9.scm:
  2881:28 11 (_ #<mutex 1f5ff80>)
In ice-9/threads.scm:
    390:8 10 (_ _)
In ice-9/boot-9.scm:
  3171:20  9 (_ #<mutex 1f5ff80>)
   2312:4  8 (save-module-excursion #<procedure 21baf90 at ice-9/boot-9.scm:3172:21 ()>)
  3191:26  7 (_)
In unknown file:
           6 (primitive-load-path "ice-9/peg" #<procedure 20c2c20 at ice-9/boot-9.scm:3178:37 ()>)
In ice-9/peg.scm:
     20:0  5 (_)
In ice-9/boot-9.scm:
   3032:4  4 (define-module* _ #:filename _ #:pure _ #:version _ #:imports _ #:exports _ #:replacements _ # _ # _ \u2026)
  3045:24  3 (_)
   222:17  2 (map1 (((ice-9 peg codegen)) ((ice-9 peg string-peg)) ((ice-9 peg simplify-tree)) ((ice-9 peg #)) #))
   2961:6  1 (resolve-interface _ #:select _ #:hide _ #:prefix _ #:renamer _ #:version _)
In unknown file:
           0 (scm-error misc-error #f "~A ~S" ("no code for module" (ice-9 peg codegen)) #f)

ERROR: In procedure scm-error:
no code for module (ice-9 peg codegen)
--8<---------------cut here---------------end--------------->8---

Thoughts?

Thank you!

Ludo’.