[reactive] Bug fixes in progress

[Svein Ove Aas]

On Fri, May 29, 2009 at 6:17 PM, Isaac Dupree
<ml at isaac.cedarswampstudios.org> wrote:
> Svein Ove Aas wrote:
>>
>> Certain people (*cough*quicksilver*cough*) thought it'd be nice of me
>> to inform you of what I've been up to the last few days re: reactive
>> bug-fixing, so here's an update.
>>
>> - I've rewritten unamb, fixing all the issues I've managed to find
>> (relating to nested unamb invocations, mostly) and having it throw a
>> BothBottom exception if both values are a finite bottom.
>
> Why not choose one of the two existing bottoms and throw that?  It'd be like
> the semantics of Haskell's imprecise exceptions anyway...
>
Basically, so it's easier to tell when an exception originates in
Unamb, for debugging Reactive.

Your suggestion has merit, but I think it'd be better to wrap *both*
the exceptions inside BothBottom instead.

>> I'm now trying to fix the joinE/>>=/mappend problems, but that might
>> take a while. Meanwhile, more eyes on the rewritten code is better, so
>> I've attached it; there hasn't been an official release yet.
>
> okay I read it!  I just have some questions / suggestions to comment things
> better in the code, currently
>
>> unamb = ...
>>    where retry act = act `catch`
>
> the definition of retry here makes almost no sense to me... but as I
> understand it, it's completely an RTS hack.  First I'd put a comment saying
> so, and then explain exactly what problem it avoids and how it does that,
> with more comments.
>
Yes indeed. I'll have a go right away:
- When an unsafePerformIO action is aborted (due, presumably, to an
exception either thrown by or thrownTo the thread running it), the RTS
puts the entire execution context of the IO code on ice, so if the
closure is later re-entered no work will have been lost, and actions
aren't executed twice unless the entire thing is executed twice. The
implementation is somewhat involved, but this is the Right Thing; it
makes the semantics of unsafePerformIO nearly identical to ordinary
pure code with regards to exceptions.
- If an unsafePerformIO action uses throw/throwIO itself, once the
unpause happens it'll just throw the exception again. Nice and
predictable.
- However, if we catch an exception in race, we clean up by killing
the threads. This messes up the entire pause/unpause mechanic - it
might be disabling it. I haven't examined it very deeply, as it in any
case doesn't do what we want.

Therefore, after killing the subordinate threads in race, the
exception propagates further to the handler in unamb, which catches
all possible exceptions. This exception handler proceeds to re-throw
the exception; inside an unblock, as exception handlers otherwise
block asynchronous exceptions until they're done.

Importantly, it does this by first getting the ID of the current
thread, and then throwTo'ing the thread instead of just throwIO'ing
the exception. This means that, once the throwTo is re-executed after
the closure is later unpaused, it'll try rethrowing the exception to
*another* (now dead, hopefully) thread, and then retry the amb call.

Now, the ordinary case for an exception in unamb is that some other
unamb invocation is killing the thread due to already having found a
non-bottom value..

...I just figured out a potential bug here, actually. If this isn't
the case - if this is a top-level unamb invocation, not something
forkIO'd by race - then the exception might get caught and the /same
thread/ might decide to retry the same unamb closure later, which
would throw the exception to that thread again... great.

Okay. It's fixable, by having unamb always execute in a separate
thread entirely.. might lend itself to cleaner code, too. I'll look
into it.

> in "race": How does your current version kill descendent threads any better
> than "Simple version"?
> Oh I see it puts the `finally` kill-the-threads in, around the read-the-mvar
> ("descentent" just means the two threads that "race" spawns, not the threads
> spawned by those two threads?).  But I'm not sure what the *extra*
> complication is for, that's beyond "Simple version"'s complexity?  Is it
> just in order to be able to throw BothBottom at the right time (a worthy
> goal)?  I also don't understand why putCatch wants to prevent certain kinds
> of exceptions from escaping the thread? (Would they emit into the
> calling-thread that `race` runs in? or what is the difference?  How did you
> choose the three or four things to catch; and what is wrong with
> NonTermination?)
>
I didn't pick what exceptions to catch, or decide that NonTermination
is bad; that's all conal's doing. reactive seems to work better with
that code in. I'm not touching it. *cargo-cults*

About my implementation of race - the major worry is handling
asynchronous exceptions correctly, where the thread is killed between
starting the descendant threads and setting up an exception handler to
kill them. That's what the block/unblock construct is for.

Also, my version has the descendants inform their creator of finite
bottoms, meaning it can throw BothBottom when they're both dead.

-- 
Svein Ove Aas