Re: [Chicken-hackers] pending patches

[Alan Post]

On Mon, Nov 14, 2011 at 01:51:47PM +0100, Felix wrote:
> > 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

This is all fine, then.  I don't have any additional feedback.

-Alan
-- 
.i ma'a lo bradi cu penmi gi'e du