Re: srfi-18 requirements

[Neil Jerram <neil@ossau.uklinux.net>]

"Julian Graham" <joolean@gmail.com> writes:

> Okay, find attached a patch against HEAD containing the aforementioned
> changes to the core for supporting SRFI-18.  I'm still looping my test
> code, but I thought I should get something out to you guys this
> evening.  In addition to the code changes, the patch includes relevant
> Changelog, doc, and threads.test updates.  Let me know what you think.

Looking good!  Many thanks for your continuing work on this, and sorry
for my delay (once again!) in reviewing.  I have a few comments, as
follows.

>  @c begin (texi-doc-string "guile" "join-thread")
> -@deffn {Scheme Procedure} join-thread thread
> +@deffn {Scheme Procedure} join-thread thread [timeout]
>  @deffnx {C Function} scm_join_thread (thread)
> +@deffnx {C Function} scm_join_thread_timed (thread, timeout)

Didn't we agree to add a timeout-val parameter here?

> +static scm_t_timespec
> +scm_to_timespec (SCM t)

For static functions it's nice to omit the scm_ prefix, because they
don't need it, and it makes it clearer to the casual reader that
they're not part of the API.

Also, can the signature be void to_timespec (SCM t, scm_t_timespec *),
to avoid relying on support for struct return?

> -SCM_DEFINE (scm_join_thread, "join-thread", 1, 0, 0,
> -	    (SCM thread),
> +SCM scm_join_thread (SCM thread)
> +{
> +  return scm_join_thread_timed (thread, SCM_BOOL_F);

You should use SCM_UNDEFINED to indicate an absent parameter, rather
than SCM_BOOL_F.

> +}
> +#undef FUNC_NAME

Last #undef line is extraneous.

> +
> +SCM_DEFINE (scm_join_thread_timed, "join-thread", 1, 1, 0,
> +	    (SCM thread, SCM timeout),

What about the timeout_val parameter ...

>  "Suspend execution of the calling thread until the target @var{thread} "
>  "terminates, unless the target @var{thread} has already terminated. ")
> -#define FUNC_NAME s_scm_join_thread
> +#define FUNC_NAME s_scm_join_thread_timed
>  {
> +  int timed_out = 0;
>    scm_i_thread *t;
> -  SCM res;
> +  scm_t_timespec ctimeout, *timeout_ptr = NULL;
> +  SCM res = SCM_BOOL_F;
>  
>    SCM_VALIDATE_THREAD (1, thread);
>    if (scm_is_eq (scm_current_thread (), thread))
> @@ -1005,11 +1068,23 @@
>    t = SCM_I_THREAD_DATA (thread);
>    scm_i_scm_pthread_mutex_lock (&t->admin_mutex);
>  
> +  if (! SCM_UNBNDP (timeout))
> +    {
> +      ctimeout = scm_to_timespec (timeout);
> +      timeout_ptr = &ctimeout;
> +    }
> +
>    if (!t->exited)
>      {
>        while (1)
>  	{
> -	  block_self (t->join_queue, thread, &t->admin_mutex, NULL);
> +	  int err = block_self (t->join_queue, thread, &t->admin_mutex, 
> +				timeout_ptr);
> +	  if (err == ETIMEDOUT)
> +	    {
> +	      timed_out = 1;

... which I would expect to be assigned to res here.

> +	      break;
> +	    }
>  	  if (t->exited)

Do res = t->result here, rather than below, to make clear that it goes
with the t->exited case?

> +static SCM
> +fat_mutex_lock (SCM mutex, scm_t_timespec *timeout, int *ret)
>  {
>    fat_mutex *m = SCM_MUTEX_DATA (mutex);
> +
>    SCM thread = scm_current_thread ();
> -  char *msg = NULL;
> +  scm_i_thread *t = SCM_I_THREAD_DATA (thread);
> +
> +  SCM err = SCM_BOOL_F;
> +
> +  struct timeval current_time;
>  
>    scm_i_scm_pthread_mutex_lock (&m->lock);
>    if (scm_is_false (m->owner))
> -    m->owner = thread;
> +    {
> +      m->owner = thread;
> +      scm_i_pthread_mutex_lock (&t->admin_mutex);
> +      if (scm_is_null (t->mutexes))
> +        t->mutexes = scm_list_1 (mutex);
> +      else
> +        t->mutexes = scm_cons (mutex, t->mutexes);

Just "t->mutexes = scm_cons (mutex, t->mutexes);" is sufficient for
both cases.

> +      scm_i_pthread_mutex_unlock (&t->admin_mutex);
> +      *ret = 1;
> +    }
>    else if (scm_is_eq (m->owner, thread))
>      {
>        if (m->level >= 0)
>  	m->level++;
>        else
> -	msg = "mutex already locked by current thread";
> +	err = scm_cons (scm_misc_error_key,
> +			scm_from_locale_string ("mutex already locked by "
> +						"current thread"));
> +      *ret = 0;
>      }
>    else
>      {
> +      int first_iteration = 1;
>        while (1)
>  	{
> -	  block_self (m->waiting, mutex, &m->lock, NULL);
> -	  if (scm_is_eq (m->owner, thread))
> -	    break;
> -	  scm_i_pthread_mutex_unlock (&m->lock);
> -	  SCM_TICK;
> -	  scm_i_scm_pthread_mutex_lock (&m->lock);
> +	  if (scm_is_eq (m->owner, thread) || scm_c_thread_exited_p (m->owner))
> +	    {
> +	      scm_i_pthread_mutex_lock (&t->admin_mutex);
> +	      if (scm_is_null (t->mutexes))
> +		t->mutexes = scm_list_1 (mutex);
> +	      else
> +		t->mutexes = scm_cons (mutex, t->mutexes);

Same again here.

> +	  else if (!first_iteration)
> +	    {
> +	      if (timeout != NULL) 
> +		{
> +		  gettimeofday (&current_time, NULL);
> +		  if (current_time.tv_sec > timeout->tv_sec ||
> +		      (current_time.tv_sec == timeout->tv_sec &&
> +		       current_time.tv_usec * 1000 > timeout->tv_nsec))
> +		    {
> +		      *ret = 0;
> +		      break;
> +		    }

Is timeout an absolute time, or relative to when join-thread was
called?  Before getting to this code, I thought it was relative - but
then I don't see how the code above can be correct, because it is
comparing against the absolute gettimeofday() ...?

> -SCM_DEFINE (scm_lock_mutex, "lock-mutex", 1, 0, 0,
> -	    (SCM mx),
> +SCM scm_lock_mutex (SCM mx)
> +{
> +  return scm_lock_mutex_timed (mx, SCM_BOOL_F);

Should be SCM_UNDEFINED.

> -static char *
> -fat_mutex_unlock (fat_mutex *m)
> +static void
> +fat_mutex_unlock (SCM mx)
>  {
> -  char *msg = NULL;
> -
> +  fat_mutex *m = SCM_MUTEX_DATA (mx);
>    scm_i_scm_pthread_mutex_lock (&m->lock);
> -  if (!scm_is_eq (m->owner, scm_current_thread ()))
> +  if (m->level > 0)
> +    m->level--;
> +  else 

It looks like there is a significant change to the semantics here: any
thread can unlock a mutex, not just the thread that locked it.  Is
that the intention, or am I misunderstanding?

> +static int
> +fat_cond_timedwait (SCM, SCM, const scm_t_timespec *);
>  
> -  return msg;
> +SCM scm_unlock_mutex (SCM mx)
> +{
> +  return scm_unlock_mutex_timed (mx, SCM_UNDEFINED, SCM_UNDEFINED);
>  }
>  
> -SCM_DEFINE (scm_unlock_mutex, "unlock-mutex", 1, 0, 0,
> -	    (SCM mx),
> +SCM_DEFINE (scm_unlock_mutex_timed, "unlock-mutex", 1, 2, 0,
> +	    (SCM mx, SCM cond, SCM timeout),
>  "Unlocks @var{mutex} if the calling thread owns the lock on "
>  "@var{mutex}.  Calling unlock-mutex on a mutex not owned by the current "
>  "thread results in undefined behaviour. Once a mutex has been unlocked, "
> @@ -1240,18 +1358,39 @@
>  "lock.  Every call to @code{lock-mutex} by this thread must be matched "
>  "with a call to @code{unlock-mutex}.  Only the last call to "
>  "@code{unlock-mutex} will actually unlock the mutex. ")
> -#define FUNC_NAME s_scm_unlock_mutex
> +#define FUNC_NAME s_scm_unlock_mutex_timed
>  {
> -  char *msg;
> +  SCM ret = SCM_BOOL_T;
> +
>    SCM_VALIDATE_MUTEX (1, mx);
> -  
> -  msg = fat_mutex_unlock (SCM_MUTEX_DATA (mx));
> -  if (msg)
> -    scm_misc_error (NULL, msg, SCM_EOL);
> -  return SCM_BOOL_T;
> +  if (! (SCM_UNBNDP (cond)))
> +    {
> +      SCM_VALIDATE_CONDVAR (2, cond);
> +      scm_t_timespec cwaittime, *waittime = NULL;
> +
> +      if (! (SCM_UNBNDP (timeout)))
> +	{
> +	  cwaittime = scm_to_timespec (timeout);
> +	  waittime = &cwaittime;
> +	}
> +      if (! fat_cond_timedwait (cond, mx, waittime))
> +	ret = SCM_BOOL_F;
> +    }

Call scm_timed_wait_condition_variable() here, instead of duplicating
the code?

Actually, that strongly says to me that we don't need the `cond' part
of this API to be implemented in C.  Can we move that to the SRFI-18
Scheme code, and leave the C API as a plain unlock-mutex operation?

Regards,
       Neil