SodiumFRP / sodium

Sodium - Functional Reactive Programming (FRP) Library for multiple languages
http://sodium.nz/
Other
848 stars 138 forks source link

Fix LazyBehavior bug #122

Closed jam40jeff closed 6 years ago

jam40jeff commented 6 years ago

Lazy cells (and behaviors) have the purpose of delaying their initial value computation such that they work with loops. For example, give a Stream<int> s and a CellLoop<int> c, the following will work:

Cell<int> cell = s.HoldLazy(c.SampleLazy());

Most (if not all) primitives use HoldLazy such that they will work with loops.

However, currently the initial value for the lazy cell is only computed when it is first sampled. This can be problematic if the initial value of a cell is another cell. For example, the following should produce the value 1, but will instead currently produce the value 0:

int lastValueSeen = -1;
StreamSink<int> s = new StreamSink<int>();
Cell<Cell<int>> c = Cell.Constant(s).Map(v => v.Hold(0));
s.Send(1);
Cell<int> cell = c.SwitchC();
cell.Listen(v => lastValueSeen = v);

The problem is that the inner cell is created during the SwitchC call rather than the moment the transaction created by the line Cell.Constant(s).Map(v => v.Hold(0)); ends, and it therefore misses the sending of the value 1.

Lazily created cells and behaviors should thus have their values created at the end of the transaction in which they were created instead of the first time they are sampled.

jam40jeff commented 6 years ago

This bug was caught when debugging the issue @dakom raised about initial cells within cells losing not being listened to correctly after SwitchC. That issue was caused by SampleLazy computing its value during the "last" phase of the transaction. A new "sample" phase was added as part of the "prioritized loop" to fix this. I would propose using this same phase to compute the values for LazyBehavior as well.

the-real-blackh commented 6 years ago

Thank you for finding this issue! I'm keen to fix it in all versions. I can see that it's fixed in

73cbcf99919ad54c006e425a0c0e731dfbc8593c c1b2aa000aa84a0288e1e29dacca5fc9889c4530 523c27612212a4fff430f7e0481ce191dfb54128 5a3504fd63a71098cbe45a343531a5effbce10f2

Have I missed any?

jam40jeff commented 6 years ago

@the-real-blackh Sorry, I should have clarified. Although I found this issue while testing the bug @dakom found, that issue was separate. I should open an issue for that one as well (although it has already been fixed in the C# code there was no issue created for it). This is a slightly different issue that isn't fixed by the code in the 4 revisions you mentioned and has yet to be fixed. I have a potential fix waiting on my machine, but I want to do some performance profiling first to make sure it's the right solution to the problem.

the-real-blackh commented 6 years ago

I'll rename the LazyBehaviour bugs that I've raised to "Apply in SwitchC". Is that correct?

jam40jeff commented 6 years ago

No, the LazyBehavior bug is new (and yet to be fixed). I created a new issue for the commits you mentioned called Fix SampleLazy bug. That is the bug which has already been fixed in C# (for which the commits say something about Apply in SwitchC, although that was just one manifestation of the bug and not the root cause).

Both bugs should be fixed in the other languages, though.

jam40jeff commented 6 years ago

Also, I should mention that although the title refers to LazyBehavior, this bug will still exist in the other languages in the LazyCell class even if Behavior has not yet been added.