jam40jeff
commented
8 years ago I believe this would open the door to memory leaks. I am actually not a fan of exposing ListenWeak in the public API (interestingly enough, it's only exposed for Stream<T> and not for Cell<T>) because I think users should always have to call Unlisten (or Dispose in C# will have the same effect) to deregister the listener.
However, the problem is with listeners created by the primitives themselves. The primitives return a new Stream<T> or Cell<T> which may be disposed, which will in turn deregister the implicit listener they created. However, these primitives are often chained, which means user code never gets a reference to them and they go out of scope very quickly. The only thing holding a reference to them is the listener, stream, or cell at the end of the chain. When the object at the end of the chain is freed up by calling Unlisten, the garbage collection is free to flow up the chain until we reach another stream or cell that is being held on to by user code because of the WeakReferences.
Here is an example of where I believe there would be a problem if WeakReference was not used:
public class Test
{
private readonly Stream<int> stream = Stream.Never<int>();
private IListener listener;
public void StartListenTest()
{
this.listener = stream.Map(_ => Unit.Value).Listen(_ => Console.WriteLine("It fired!"));
}
public void StopListenTest()
{
this.listener.Unlisten();
}
public static void RunTest()
{
Test test = new Test();
test.StartListenTest();
test.StopListenTest();
// At this point, test.listener should be cleaned up, but the listener created by the call to Map
// will not be unless it was created with a WeakReference because test.stream will still hold
// a reference to it. There is no way to deregister that listener until test.stream (and thus,
// test itself) goes out of scope.
Thread.Sleep(5000);
// now everything will get cleaned up (eventually when the GC gets to it) when this method
// exits and the test variable goes out of scope
}
}The only way I can see where this would work is if users were forced to dispose of all intermediate streams and cells returned by primitives as follows:
public class Test
{
private readonly Stream<int> stream = Stream.Never<int>();
private Stream<Unit> mappedStream;
private IListener listener;
public void StartListenTest()
{
this.mappedStream = stream.Map(_ => Unit.Value);
this.listener = this.mappedStream.Listen(_ => Console.WriteLine("It fired!"));
}
public void StopListenTest()
{
this.listener.Unlisten();
this.mappedStream.Dispose();
}
public static void RunTest()
{
Test test = new Test();
test.StartListenTest();
test.StopListenTest();
// At this point, everything will be cleaned up, but we have disallowed chaining of primitives
// and forced users to track when the intermediate streams and cells are not in use
// anymore and are available for disposal.
Thread.Sleep(5000);
// everything has already been cleaned up
}
}However, I believe this would place undue burden on the users and would lead to unexpected memory leaks when they do not adhere to this unreasonable requirement.
The way the code currently works (with the usage of WeakReference), the intermediate Stream<Unit> in the first example is available for GC as soon as this.listener.Unlisten(); is called.
ziriax
the-real-blackh
ritschwumm
HeinrichApfelmus
Mainly for fun and to understand the code-base in depth, I started porting the c# version of sodium to Typescript.
Now, the Node class uses a WeakReference to the action.
Unfortunately Javascript does not have weak refs yet... The ES6 WeakMap is also not suitable for implementing weak even listeners.
https://esdiscuss.org/topic/weak-event-listener
Would it be ok to require an explicit unlisten call to cleanup memory, as in RX?