Re: srfi-18 requirements

[Ludovic Courtès]

Hi Julian,

Overall, the patch looks good to me and your explanations were helpful.
The caveat is that I'm not familiar with SRFI-18 and not too comfortable
with these parts of the pthread API that you're wrapping, so I may well
overlook a few things here and there.  I think we'll have to stress-test
the thing on NetBSD since it's good a catching thread-related
programming errors.

My comments below are mostly cosmetic.  Once you're done with them,
could you please provide a ChangeLog entry?

Then we'll need to update the doc of the core API, and also add
documentation of the SRFI module itself (the custom has been to somewhat
duplicate SRFIs in the manual so that we have all documentation in one
place).


"Julian Graham" <address@hidden> writes:

> * scm_join_thread, which now calls scm_join_thread_timed, will rethrow
> any uncaught exceptions thrown by the terminated thread

`threads.test' doesn't need to be updated because threads don't throw
exceptions, right?

> * The scm_spawn_thread call that launches the signal delivery thread
> no longer specifies a handler. No one should call scm_spawn_thread
> with a handler, because of an already-present deadlock in 1.8.x -- in
> a multithreaded context, a guile mode thread (i.e., one that has
> locked its heap mutex) may attempt to enter a critical section in
> eval.i.c at the same time a non-guile mode thread created by
> scm_spawn_thread is within a critical section in make_jmpbuf while
> setting up a catch handler and attempts to do a GC

I don't quite get this one.  Could you illustrate the problem
step-by-step with a simple scenario?

The value of HANDLER in `scm_spawn_thread ()' doesn't seem to affect
critical-section locking.

> --- threads.c 20 Oct 2007 11:09:58 -0000      1.90
> +++ threads.c 29 Oct 2007 13:15:34 -0000
> @@ -1,4 +1,4 @@
> -/* Copyright (C) 1995,1996,1997,1998,2000,2001, 2002, 2003, 2004, 2005, 
> 2006, 2007 Free Software Foundation, Inc.
> +/* Copyright (C) 2007 Free Software Foundation, Inc.

Don't do that, we must keep all years.

In `block_self ()':

> @@ -239,6 +265,7 @@
>       err = EINTR;
>        t->block_asyncs--;
>        scm_i_reset_sleep (t);
> +      scm_i_pthread_cleanup_pop (0);
>      }

Why is it needed?

> +extern scm_i_thread *scm_i_signal_delivery_thread;

Could it go in one of `{threads,scmsigs}.h'?

> +typedef struct {

Make sure to follow the GCS, i.e., put the opening brace on a line of
its own.

> -  /* Ensure the signal handling thread has been launched, because we might be
> -     shutting it down.  */
> -  scm_i_ensure_signal_delivery_thread ();
> -
>    /* Unblocking the joining threads needs to happen in guile mode
>       since the queue is a SCM data structure.  */
>    scm_with_guile (do_thread_exit, v);
>  
> +  /* Ensure the signal handling thread has been launched, because we might be
> +     shutting it down.  */
> +  scm_i_ensure_signal_delivery_thread ();
> +

Why does that need to be moved?

> +SCM_DEFINE (scm_join_thread_timed, "join-thread", 1, 2, 0, 

Scheme/C name mismatch.  I believe it effectively hides the first
`join-thread', right?

> +"Suspend execution of the calling thread until the target @var{thread} "

Indenting would be nicer.

> +"@var{thread} has already terminated. If @var{timeout_val} is specified and "

Remember spaces after periods.  :-)

> +       int err = block_self 
> +         (t->join_queue, thread, &t->admin_mutex, timeout_ptr);

Open bracket right after `block_self'.

> ;;; Time
>  current-time
>  time?
>  time->seconds
>  seconds->time

Too bad we have yet another time API...

>  current-exception-handler
>  with-exception-handler
>  raise

... and another exception API, too.

> (define (unspecified) (display ""))

We have `*unspecified*' in core Guile, which is better because it
doesn't have any side-effect, so better use it.

> (define (join-timeout-exception? obj) (eq? obj 'join-timeout-exception))
> (define (abandoned-mutex-exception obj) (eq? obj 'abandoned-mutex-exception))
> (define (uncaught-exception? obj) 
>   (and (pair? obj) (eq? (car obj) 'uncaught-exception)))
> (define (uncaught-exception-reason exc)
>   (cdr (check-arg-type uncaught-exception? exc "uncaught-exception-reason")))

I'd use pairs or records for exception objects rather than just symbols
since symbols can always be forged.  So we'd have, e.g.:

  (define uncaught-exception?
    (let ((exception-type (cons 'uncaught-exception #f)))
      (lambda (obj)
        (and (pair? obj)
             (eq? (car obj) exception-type)))))

> (define (seconds->time x)
>   (and (check-arg-type number? x "seconds->time")

Type-checking overhead may become a concern if we are to convert values
often, e.g., once after every timed-join trial.  OTOH, it's good to have
good error reporting.

Thanks,
Ludovic.