make it work with reactive-banana again

[alios]

Hi @luite!

With this patch I made reactive-banana work again. It didn't actually include your very recent thread related patch, as they produce "a uncaught excpetion: Thread aborted" on startup of my application.

But with this small patch my reactive banana code works again without any problems.

[luite]

The IS_MARKED(w) check is wrong according to the semantics (or at least it only applies to tombstoning, otherwise finalizers for unreachable weaks would never be run), plus removing the two lines marking the h$Weak objects makes it never run any finalizers.

Can you make sure that the test cases rts/weak/weak001.hs, pkg/base/weak001.hs and ghc/concurrent/t7970.hs pass before sending a pr?

I think you did spot an issue with the condition for pushing weaks to the toFinalize list, I've pushed a change with a slight modification of your change.

Can you point me to the code that gets you a thread aborted message? The change makes it clean up threads that get to the shutdownHaskellAndExit call, whereas before, threads could continue afterwards in the browser, where it's not possible to really exit.

[alios]

Hi @luite!

Thanks for the explaination and yes on the the testcases!

Found the reason for the killed thread and it totaly makes sense: The action which I call from my main to start the reactive-banana network does not block m) Which means my main simply returned, which I did not recognize before you worked on the correct shutdown ;)

I added a forever $ threadDelay 1000000 which keeps my main now alive which solves the problem of the killed thread. Although the effect stays the same, after GC the network stops doing things.

On the semantics at the tobstoning:

  4. Scan the Weak Pointer List again. If the weak pointer object is reachable
     then tombstone it. If the weak pointer object has a finalizer then move
     it to the Finalization Pending List, and mark all the heap reachable
     from the finalizer. If the finalizer refers to the key (and/or value),
     this step will "resurrect" it.

Entering step 4 we have all those weak pointers left in the (old) h$weakPointerList which keys are not reachable anymore (at least at this point in time). If the weak pointer object it self is still reachable (thats why I think the IS_MARKED(w) is important), we only unref the value (aka tombstone it) in this round of the GC, by setting the value null. We don't want the GC to eat the finalizer so we mark it on those objects where the weak pointer object it self is still reachable and submit it to the list of finalizers to run. The marking there might now also mark the key and or value if it is referenced from the finalizer. This is how I understand the "resurrect" sentence.
Those weak pointers where the key and/or value is still not marked, are completly "dead" an will be finalized by the JS GC (not referred anymore after the shims GC is done).

Btw i still think that the explicit marking of all pointers in the new h$weakPointerList makes sense (together with not marking them in the iteration over all heap objects) in this context. Because then only those weakpointers will be marked as still reachable which made it in the new h$weakPointerList - all the others should will be eaten by the JS GC as they are no longer referenced by the ``h$weakPointerList`. It is also asured that their finalizers will be run (and not GC't) because we it as reachable and also but it to the finalizers list.

After the GC is complelty done, it will returned. The application code might sill have a valid reference to the weak pointer object it self, but can't actually do anything with it anymore. but as long it has a reference, the backing object in the js world will continue to live.

So to put it back in the context of the definition i understand it like this (I added things in bold to make my point how I understand the semantics):

Scan the Weak Pointer List again.
If the weak pointer object is reachable then tombstone it.
If the weak pointer object is reachable and has a finalizer then move it to the Finalization Pending List, and mark all the heap reachable from the finalizer. If the finalizer refers to the key (and/or value),
this step will "resurrect" it.

Please correct me if I'am wrong with that.

[luite]

You removed marking all h$Weak objects in h$follow, so no weak objects were considered reachable.

Unreachable weak refs don't need to be tombstoned, since they are dead anyway, but it doesn't really hurt to still do so. The key to replicating GHC's semantics is to keep the values of weak references alive if their key is reachable, regardless of whether the weak reference.

The bold text doesn't appear in the paper (or at least the version I have), and it doesn't make sense given the semantics specified in the paper. Reachability of the weak object can't be in the condition for moving the weak object to the finalization pending list: If the weak object would just be discarded if it's unreachable, then its finalizer would never be run. This contradicts the earlier assertion that all finalizers are run exactly once during the execution of the program (this is true by approximation in GHCJS, and i suspect in GHC as well: early terminaton of the RTS may prevent some finalizers to be run. Perhaps GHCJS should do a bit more in this area and let the shutdownHaskellAndExit call wait for finalizer threads to finish)

[luite]

the h$ThreadAbortedError should never be printed by the way, are you compiling with some debug flags on?

[luite]

do you have a simpe (terminal) reactive-banana program that fails?

[luite]

As far as I know, reactive-banana 1.0 should work now. Please report any problematic programs.

[alios]

@luite @HeinrichApfelmus still have problems with my code but I was not yet able to cook it down to a small test, which reproduces that. The efect is still the same, when the gc hits the first time the reactive code stops working.

What I'am basicly doing is:

• Use ghcjs-dom to construct UI Components and attach the to the DOM at runtime.
• Attach DOM event handlers to some of them (f.e. onClick, onMouseOut) which fire events in the FRP systems.
• having a FRP network where the events flow in and in the most cases are updating stepper base Behaviors. On change of behaviors IO Handlers f.e. Update things in the rom again.

My current theory is, but as I said I wasn't able to reproduce yet, that stil something in the garbage collection is "wrong" and that something between the DOM event handler and the reactive-banana Event (fire function) is beeing garbage collected (esp. with ghcjs-dom beeing a "foreign" library).

But i will further investigate.

Btw. actualy the patch behind this pull request still makes it work again, but there is obviously a memory leak then (found my firefox on macos with a virtual memory of 12G after developing a whole day on the project ;))

[luite]

For these things it's important that you retain and release callbacks for the event handlers properly (for which there is unfortunately no convenient automatic way in JS). There could well be something wrong with the retention/scanning code related to those. I'm working on callbacks and the low level thread API today and I'll test this if I have time.

[alios]

I keep the the cleanup handlers in a big list i produce using a Writer Monad, so they should at least be somewhat referenced from my main. I also keep an pointer to the DOM Elements (although they are inserterted to the dom). But maybe you find something there. thnx!

[luite]

Are you sure these values are live in the main thread? The can easily be dead if the thread has already stopped or never uses them anymore (for example if you just have it looping).

Anyway that looks like it's going to be a bit too much guesswork. I'm going to add more testcases for callbacks etc so I might accidentally run into the problem, but otherwise i'm going to need a reasonably simple way to reproduce the problem before i can fix it.

[avieth]

I've come across a weird bug and I wonder if it's related to this. Check out this program:

{-# LANGUAGE OverloadedStrings #-}

import Control.Monad.Trans.Reader
import Control.Monad.IO.Class
import GHCJS.Types
import GHCJS.DOM
import GHCJS.DOM.Element as Element
import GHCJS.DOM.Node as Node
import GHCJS.DOM.Document as Document
import GHCJS.DOM.EventM
import Reactive.Banana.Combinators
import Reactive.Banana.Frameworks
import Debug.Trace

main = runWebGUI $ \webView -> do
    Just document <- webViewGetDomDocument webView
    Just body <- getBody document

    let networkDescription :: MomentIO ()
        networkDescription = do
            Just div <- document `createElement` (Just ("div" :: JSString))
            Just textInput <- document `createElement` (Just ("input" :: JSString))
            div `appendChild` (Just textInput)
            body `appendChild` (Just div)

            (clicks, fireClick) <- newEvent

            liftIO $ on div Element.click $ do
                         eventData <- ask
                         liftIO $ fireClick eventData

            -- Remove any one of these reactimates and it's all good.
            reactimate (undefined <$> never)
            reactimate (undefined <$> never)
            reactimate (undefined <$> never)
            reactimate (undefined <$> never)

            reactimate ((\_ -> trace "Click" (return ())) <$> clicks)

            return ()

    network <- compile networkDescription
    actuate network

    return ()

Running this, you'll see an input box. Clicking on it should print "Click", and it always does at least once. If you click very quickly, you may get 3 or 4 prints, but then it stops working. It seems there's something special about 5 reactimates; removing any one of the useless reactimates in that block of 4 gives the expected behaviour: clicking always prints "Click".

I tried it with this branch, and the problem remains. But I wonder, could this be a symptom of the same problem? What do you think?

Addendum: I flipped on -DGHCJS_TRACE_WEAK. I find that, with 4 reactimates, nothing is ever finalized. With 5, after the first click, 40 weak pointers are finalized, and then the click output stops.

Addendum 2: this branch actually does seem to fix it :) I did a ghcjs-boot pointing to this branch but I didn't verify that the gc.js file in the right .ghcjs/ directory was actually updated. It wasn't. I manually copied the file from this branch and now it works!

[achirkin]

I have exactly the same problem, and this pull request fixes it.
I have not tried to boot ghcjs, but just copied ghcjs/shims/src/gc.js as @avieth suggested.

Not sure if it is related, but the change in ghcjs/ghcjs#450 alone does not fix the problem. So I have applied both changes to my local version of the file.

I see there have been no conversation in this thread since Dec 2015. Are there any obstacles against this PR?

[luite]

I closed the PR because at least the first version got rid of all finalization, so it didn't really fix the problem but replace it with a different one that is somewhat harder to spot (memory leak).

There have been a few small fixes wrt traversal of some heap objects (small arrays and some specific closure variables). Are you running the latest version of the master branch?

If you still have a problem, can you make a small program that demonstrates it. Ideally without any dependencies outside what ghcjs-boot installs. A node.js runnable (terminal) program that just depends on reactive-banana may be usable too.

[achirkin]

Thanks, @luite !
I think ghcjs/issues/463 is related to this, so I added a snippet there.