[reactive] Bug fixes in progress

[Svein Ove Aas]

Okay. I've tested, and there is no bug.

This breaks my mental model of how unsafePerformIO actually works
somewhat. I'm going to have to chat with #ghc before I can write any
comments.

On Fri, May 29, 2009 at 8:24 PM, Svein Ove Aas <svein.ove at aas.no> wrote:
> 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
>



-- 
Svein Ove Aas