Re: thread cancellation, take 2

[Ludovic Courtès]

Hi Julian,

"Julian Graham" <address@hidden> writes:

> Alright -- I think I've got it working.  After mucking about for a bit
> with asyncs, I realized that it makes more sense to evaluate cleanup
> handlers in do_thread_exit -- and evaluation should happen there
> anyway, since pthreads says cleanup handlers get called when the
> thread exits for any reason, not just cancellation.  Duh.

Good!

> The patch I've attached adds three new public functions:
> cancel-thread, push-thread-cleanup, and pop-thread-cleanup.  API
> documentation on their behavior is included in the changes.

Thinking a bit more about it, maybe we could let users handle the list
of handlers if needed.  That is, we'd have just
`set-thread-cleanup-procedure!' (a two-argument procedure whose first
arg is a thread) and `thread-cleanup-procedure' (a one-argument
procedure); users who have cleanup code spread over several procedure
would provide a procedure that iterates over such pieces of code.  That
would keep Guile's built-in mechanisms minimal.

What do you think?

> I've never submitted a patch for Guile before, so I've likely made a
> few mistakes in formatting, etc., and I don't really know the vetting
> procedure. I hope I've gotten the main bits right, though.  Please let
> me know if there are any changes I should make.

First, you'll need to assign copyright to the FSF so that we can
incorporate your changes (I'll send you the relevant stuff off-line).
Then, you need to make sure your code follows the GNU Standards as much
as possible (a few comments follow).  Also, please add a few test cases
to `threads.test' that exercise the new API.

I think the patch adds a useful feature so, unless someone raises an
objection, I'd be OK to apply it to HEAD when you're done with the
copyright paperwork.

> +static SCM handle_cleanup_handler(void *cont, SCM tag, SCM args) {
> +  *((int *) cont) = 0;
> +  return scm_handle_by_message_noexit(NULL, tag, args);
> +  return SCM_UNDEFINED;
> +}

The second `return' is superfluous.  Also, for clarity and consistency
with the GCS, the function should rather read:

  static SCM
  cleanup_handler_error_handler (void *cont, SCM tag, SCM args)
  {
    int *continue_p = (int *) cont;

    *continue_p = 0;

    return scm_handle_by_message_noexit (NULL, tag, args);
  }

But if you agree with the change I suggested above (removing the list of
handlers), you no longer need that function: you can just use
`scm_handle_by_message_noexit ()' directly.

> +  while(!scm_is_eq(t->cleanup_handlers, SCM_EOL)) 
> +    {
> +      int cont = 1;
> +      SCM ptr = SCM_CAR(t->cleanup_handlers);
> +      t->cleanup_handlers = SCM_CDR(t->cleanup_handlers);

Please add a whitespace before opening parentheses.

> +SCM_DEFINE (scm_cancel_thread, "cancel-thread", 1, 0, 0,
> +         (SCM thread),
> +"Asynchronously force the target @var{thread} to terminate. @var{thread} "
> +"cannot be the current thread, and if @var{thread} has already terminated or 
> "
> +"been signaled to terminate, this function is a no-op.")

Better indent the doc string.

Thanks,
Ludovic.