unofficial mirror of bug-guile@gnu.org 
 help / color / mirror / Atom feed
From: ludo@gnu.org (Ludovic Courtès)
To: Mark H Weaver <mhw@netris.org>
Cc: 31878@debbugs.gnu.org
Subject: bug#31878: Module autoloading is not thread safe
Date: Mon, 22 Oct 2018 12:10:15 +0200	[thread overview]
Message-ID: <87k1manvg8.fsf@gnu.org> (raw)
In-Reply-To: <871s8j17xq.fsf@netris.org> (Mark H. Weaver's message of "Sun, 21 Oct 2018 14:16:49 -0400")

Hi Mark,

Mark H Weaver <mhw@netris.org> 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’.





  reply	other threads:[~2018-10-22 10:10 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-06-18  9:43 bug#31878: Module autoloading is not thread safe Ludovic Courtès
2018-06-18 11:11 ` Ludovic Courtès
2018-06-18 12:17   ` Ludovic Courtès
2018-08-22 23:22     ` Mark H Weaver
2018-08-23  2:18       ` Mark H Weaver
2018-08-23 13:54         ` Ludovic Courtès
2018-08-23 19:40           ` Mark H Weaver
2018-08-24  8:45             ` Ludovic Courtès
2018-10-21 18:16             ` Mark H Weaver
2018-10-22 10:10               ` Ludovic Courtès [this message]
     [not found]     ` <876002dm18.fsf@netris.org>
2018-08-23 13:51       ` Ludovic Courtès
2022-04-04 11:47 ` Calvin Heim

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://www.gnu.org/software/guile/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=87k1manvg8.fsf@gnu.org \
    --to=ludo@gnu.org \
    --cc=31878@debbugs.gnu.org \
    --cc=mhw@netris.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).