Re: srfi-18 requirements

[Neil Jerram]

"Julian Graham" <address@hidden> writes:

> Hi Neil,
>
> Thanks for your comments.

No problem.  I feel like I'm being a bit awkward, so thanks for taking
these comments in good faith.

>> 1. Some of your changes are bug fixes for the existing thread code,
>>    and not dependent on SRFI-18, so we should apply these to 1.8.x as
>>    well as to HEAD.  Can you pull these out into a separate patch?
>
> Sure.
>
>
>> 2. I don't think it's clear what the overall effect of the SRFI-18
>>    enhancement will be.  Is it your intention that Guile will then
>>    implement SRFI-18 behaviour by default?  Or that it will be an
>>    option, alongside the existing thread behaviour?
>>
>>    Based on the current patch, I think you intend the latter, and that
>>    the choice of existing/SRFI-18 behaviour is made by what procedures
>>    the calling code uses, which in turn depends on whether that code
>>    has done (use-modules (srfi srfi-18)).  Right?
>
> Yes, that's right.  The purpose of the C component of the patch is to
> add to and extend Guile's core API in a way that is consistent with
> the existing core code (in terms of function names and arguments) but
> which allows the Scheme component of the patch to cleanly (i.e,
> relying mainly on primitive functions) implement SRFI-18 on top of it.

Thanks.  I still need to persuade myself that the dividing lines are
in the right places - am now doing a detailed review of the patch to
work that out.

>>    What then happens if a SRFI-18 procedure is called on a
>>    non-SRFI-18-created thread, or vice versa?
>
> Nothing significantly different, in most cases.  The main differences
> between threads created by core code and by SRFI-18's make-thread are
> in the way they start up (SRFI-18 threads must be explicitly started)
> and in the way exceptions are handled (SRFI-18 threads have a
> top-level exception handler that marks exceptions for rethrow  in
> joining threads).  As far as I can see, the worst case would be a lost
> exception thrown from an SRFI-18 call in a non-SRFI-18 thread -- but
> that's Guile's existing behavior.

Thanks.

>>    (Is there already a list somewhere of differences between existing
>>    and SRFI-18 behaviour?  That would help my understanding, at
>>    least.)
>
> I can prepare one.

Don't bother for now; let's see if all is sufficiently clear after
detailed review.

>> 3. I think it's important that existing thread behaviour continues to
>>    work exactly as it does before your enhancement (modulo bug fixes,
>>    of course), so I'd prefer if the code changes could be structured
>>    in such as way as to make it obvious that this will be the case.
>>    Perhaps this is impossible, in which case we have to rely on
>>    review, but if there is anything that could be done here, that
>>    would be good.
>
> Agreed, except that in a few cases the patch introduces changes to
> existing behaviors that were previously "undefined" -- for example,
> unlocking a mutex from outside the thread that originally locked it.
> SRFI-18 addresses this explicitly and describes the expected behavior.

But unfortunately it is possible for people to have inadvertently
depended on unspecified behaviour.  So we still need to consider those
cases in our analysis of back-compatibility implications.

>> Why is Thread 2 still in the critical section when it calls
>> make_jmpbuf?  That strikes me as the problem here.
>
> I should've been clearer -- SCM_CRITICAL_SECTION_START occurs *within*
> make_jmpbuf.  As to why this is necessary, I must confess I'm not well
> enough versed in the particulars of Guile's jmp and async code... it
> may indeed be anachronistic, but it'll take me some time to trace
> through things to see if that's the case.  Anyone out there happen to
> know?

I think the use of a critical section here is just nonsense, because
all the data being touched is local.  Here is the code; can anyone
else see a reason for a critical section here?

static SCM
make_jmpbuf (void)
{
  SCM answer;
  SCM_CRITICAL_SECTION_START;
  {
    SCM_NEWSMOB2 (answer, tc16_jmpbuffer, 0, 0);
    SETJBJMPBUF(answer, (jmp_buf *)0);
    DEACTIVATEJB(answer);
  }
  SCM_CRITICAL_SECTION_END;
  return answer;
}

So I think the solution is just to remove the SCM_CRITICAL_SECTION_*
lines.

Would that also mean that you could revert the change to the spawning
of signal_delivery_thread()?  As this stands, I'm concerned that
you're introducing an observable difference.  For example: program
calls sigaction specifying a signal handler and a specific thread;
later that thread exits, then the signal is raised.  I believe this
will cause signal_delivery_thread/scm_system_async_mark_for_thread to
raise a "thread has already exited" exception, which is currently
reported.

(It's also nonsense to allow the signal delivery thread to exit in
such a case... but that's another matter.)

>> See e.g. handling of scm_srfi1_map in srfi/srfi-1.c.  Would that work
>> for the SRFI-18 extensions?
>
> I take it you're talking about "map" and "map-in-order" both using
> scm_srfi1_map via SCM_GPROC and SCM_REGISTER_PROC, respectively?  Yes,
> that looks feasible.

No, I meant how the srfi-1 map (defined by module (srfi srfi-1)) is
distinct from the core Guile map (defined by (guile)).

In other words, the suggestion is that the SRFI-18 implementation of
join-thread would not be a core binding, but would come from (srfi
srfi-18).

>> Note that this would imply a separate SRFI-18 library, which gets
>> loaded when the srfi-18 module is first used.  So clearly this is
>> related to the points above code structure and separating existing and
>> SRFI-18 behaviour.
>
> Not necessarily, given how close the core behavior already is to
> SRFI-18 (unless I'm misunderstanding) -- that is to say, loading the
> SRFI-18 module doesn't change the behavior of any existing thread
> functions in either Scheme or C.  Even in cases where new
> SRFI-18-supporting functions have been added that have Scheme exposure
> (e.g., scm_lock_mutex_timed -> "lock-mutex-timed"), SRFI-18 wraps them
> in new names that don't conflict with existing bindings
> ("mutex-unlock!").

I'll follow up on this after full review.

> Shall I go ahead and prepare bugfix patches against 1.8.x and HEAD for
> the non-SRFI-18 stuff?

Yes, please.  Can you include the make_jmpbuf fix discussed above,
assuming that it passes all your tests?

Thanks,
     Neil