Re: [Chicken-hackers] Scheduler fdset assert patch

[Jonathan Chan]

Hi Peter,

Thanks for the comments!

Here is a revised version of the patch (I came up with yet more edge
cases...) that has a very long comment fully explaining my reasoning.
I've also ported the scheduler tests I made to pure CHICKEN so they
should easily run on Windows and added some more tests for the new edge
case. mutex-unlock! also didn't seem to be clearing the block object
after unlocking, so I added one line to do that.

Best regards,
-- 
  Jonathan Chan
  address@hidden

On Sat, Jul 11, 2015, at 09:56 AM, Peter Bex wrote:
> On Fri, Jul 10, 2015 at 08:45:02PM -0400, Jonathan Chan wrote:
> > Hello all,
> > 
> > Here is the scheduler assert patch, test cases, and integration with the
> > test suite combined into a single patch! After a bit more testing with
> > code that used a lot of threads, I ended up looking at how make-thread
> > uses ##sys#make-thread and added some missing code to the patch's usage
> > of ##sys#make-thread.
> 
> Hi Jonathan!
> 
> First off, thanks for your efforts to fix this nasty old bug.  This is
> a very improvement to CHICKEN.
> 
> I haven't had time to study your patch in detail yet, but here are a
> few first comments:
> 
> - The test expects the user to compile sink.c manually, this means a
>    simple "make check" will fail.  Compilation should be added to the
>    test suite for it to work without manual intervention.
> - runtest.bat in Windows doesn't have the new test.
> - The new files need to be added to distribution/manifest or they won't
>    be included in distribution tarballs (we all forget this one :P)
> - In the original version of scheduler.scm, there's a line that says
>     (when (pair? p) ; (FD . RWFLAGS)? (can also be mutex or thread)
>   You seem to have replaced this with an unconditional cdr in the foldl;
>   is that intentional (ie, was the comment wrong)?
> - The foldl is rather complicated.  Could you add some comments to the
>    lambda to explain what it's doing, exactly?
> - Speaking of comments, maybe adding a high-level comment that explains
>    why the handler needs to be wrapped would be a good idea as well.
>    The scheduler is nasty tricky code, which really needs more comments.
>    Of course I understand that you've been given code without comments
>    and that's a problem with the existing codebase, but now that you have
>    all the info in your mind, it would be great to add back some comments
>    to make it easier for the next person who needs to touch this code.
> 
> Cheers,
> Peter
> Email had 1 attachment:
> + signature.asc
>   1k (application/pgp-signature)

scheduler-interrupts-complete.diff
Description: Binary data