Re: [Chicken-hackers] pending patches

[Felix]

> I have tried to devote some attention to this one and found that I
> am still no good at modifying Chicken and then successfully running
> the result.  I do have some comments on the patch, which I had hoped
> to provide as a patch, but since I seem unable to do that, I will
> provide them more informally and please beg your forgiveness.

Thanks for your comments, Alan. I appreciate your feedback and will
try to give more information about the changes.

> 
> diff --git a/library.scm b/library.scm
> index 8075c2f..9b65ad2 100644
> --- a/library.scm
> +++ b/library.scm
> @@ -1736,9 +1746,17 @@ EOF
>  
>  (define ##sys#stream-port-class
>    (vector (lambda (p)                  ; read-char
> -           (##core#inline "C_read_char" p) )
> +           (let loop ()
> +             (let ((c (##core#inline "C_read_char" p)))
> +               (if (eq? -1 c)          ; EINTR
> +                   (##sys#dispatch-interrupt loop)
> +                   c))))
>           (lambda (p)                   ; peek-char
> -           (##core#inline "C_peek_char" p) )
> +           (let loop ()
> +             (let ((c (##core#inline "C_peek_char" p)))
> +               (if (eq? -1 c)          ; EINTR
> +                   (##sys#dispatch-interrupt loop)
> +                   c))))
>           (lambda (p c)                 ; write-char
>             (##core#inline "C_display_char" p c) )
>           (lambda (p s)                 ; write-string
> 
> There are a whole class of functions that might return EINTR, and
> this set, read/peek, seem arbitrarily chosen to me.  fcntl,
> nanosleep, wait, close amongst others can return EINTR.  In my
> program that does signal handling, I catch this exception in the
> Scheme code and restart the exception.  This is working for me,
> so I don't strictly need the library to do it.  if it's going
> to, however, it seems like it should do it everywhere it can happen.
> (Other places in the code also do this, please consider this my
> representative comment for al of those places.)

You are of course right. The Right Thing would be to wrap all
calls to POSIX functions into EINTR handling code (if they can be
interrupted). I chose those functions initially that have a 
potential to hang for a certain time, which means read/input
functions. I think it is ok to start with these to not disrupt
too much code at the same time.

> 
> 
> diff --git a/library.scm b/library.scm
> index 8075c2f..9b65ad2 100644
> --- a/library.scm
> +++ b/library.scm
> @@ -4344,10 +4373,23 @@ EOF
>  
>  (define ##sys#context-switch (##core#primitive "C_context_switch"))
>  
> +(define ##sys#signal-vector (make-vector 256 #f))
> +
>  (define (##sys#interrupt-hook reason state)
> 
> 
> It turns out that there is a variable, _NSIG, defined in signal.h.
> I am pretty certain this variable is stable across Solaris, Linux,
> and BSD.  It also is a value much closer to 32 than it is to 256.
> 

(Side note: I can recall several occasions where some definitions
were believed to be "pretty certain" consistent across platforms,
when in fact they were not)

> While 256 is nice and arbitrarily high, the actual size of this
> vector is properly defined in _NSIG.  I think this vector should
> use that value rather than this magic number.

I agree that 256 is probably too high.

> Rather than storing a queue of pending signals, we are permitted to
> store a masked set of pending signals, mimicking the same structure
> as it appears in the kernel.  When a signal arrives, we can use the
> signal number as an index and mark the signal as pending.  Then we
> can iterate over the signal maskset and deliver those signals.  It
> is possible that we'll get a signal delivered twice or more and only
> deliver it to our Scheme code once, but the kernel already behaves
> this way, and we will always deliver *a* signal.  In my testing, I
> was seeing SIGCHLD delivered 32 times for 256 child processes.  30
> or 31 signals instead of 32 is truly not a problem.

You are right, of course, but on the other hand it might be preferable
to deliver as many signals as possible (even if not all signals can).

> 
> There is nothing at all wrong with this implementation as it appears
> in the patch, save for the theoretically potential case of receiving
> more signals than we have space in the pending queue.  I don't think
> we could actually exceed 100, because of kernel will discard
> duplicates and there just aren't enough signals to make it happen.
> So I'm stating that we can get away with a weaker guarantee (a mask
> rather than a queue) and still be every bit as reliable as we are now.

In between the return of the signal handler and the dispatching of
signals, any number of signals may come through. I still think it might
be better to catch as many as possible.


cheers,
felix