Drop behavior for StopIf

[A-Manning]

StopIf should drop the underlying signal once the stop condition is fulfilled, instead of just not polling it any more.

[Pauan]

It does drop the underlying Signal.

When poll_change returns Poll::Ready(None) that means the Signal has ended, which means there will never be any future changes.

That tells the downstream consumer that it can safely Drop the Signal, because the Signal will never change.

---

Dynamic methods like flatten will drop the Signal when it returns Poll::Ready(None):

https://github.com/Pauan/rust-signals/blob/2a91d9c14dba857e3888f8764f3fa0883e8575fe/src/signal/signal.rs#L1611

https://github.com/Pauan/rust-signals/blob/2a91d9c14dba857e3888f8764f3fa0883e8575fe/src/signal/signal.rs#L1623

---

The for_each method will also drop the Signal if it returns Poll::Ready(None), but this is done in a bit of an indirect way.

In order to understand it, you first need to understand how Rust implements Futures:

http://aturon.github.io/blog/2016/09/07/futures-design/

That article is a little outdated, but the concepts are still correct. Rust automatically inlines structs and methods. So let's say you create a chain of various Signal methods:

some_signal
    .map(...)
    .filter(...)
    .dedupe()
    .for_each(...)

Each one of those methods returns a struct, which contains the internal state that the method needs.

Rust will create a single giant struct which combines all of those structs together. And all of the methods are inlined into a single giant method.

This is excellent for performance, since there's no boxing or indirection, it just runs the code directly, and the code can access the struct state directly.

However, that giant struct is allocated onto the stack, which won't work for asynchronous stuff like Signal/Stream/Future.

So we have to Box the struct. When the giant struct is boxed it is called a "task". Because we are boxing up the entire giant struct, we only need a single box allocation for the entire chain of methods, which is very efficient for performance.

When you use a Future executor like tokio::spawn or wasm_bindgen_futures::spawn_local, it first box allocates the entire giant Future struct into a task, then it runs the task in the background, and then it deallocates the box when the Future returns Poll::Ready(()), which cleans up the entire task with just 1 operation.

This means that the entire chain of methods only needs 1 box allocation when it's spawned and 1 box deallocation when it's finished, which is wicked fast for performance.

When you use the for_each method, if the input Signal returns Poll::Ready(None), then the for_each will return Poll::Ready(()) which tells the Future executor that the Future is finished, and the executor will then deallocate the task, which cleans up the memory for the entire task.

This is how the Future/Stream/Signal system is designed: the Future/Stream/Signal returns Poll::Ready(None) or Poll::Ready(()) which indicates that it is finished, and then the consumer of the Future/Stream/Signal does the actual cleanup by dropping the Future/Stream/Signal.

[A-Manning]

Thank you for the detailed explanation! I also appreciate the answers that you have given in other issues, they are very helpful for learning how to use this crate.

I can see that the underlying signals are dropped as soon as they have polled as completed in Flatten, and therefore also eg. Switch. However this is not the same as StopIf. IIUC, StopIf is intended to be dropped by it's owner as soon as it has polled as completed. This is not the same drop behavior as Flatten. When writing a signal adapter that owns StopIf, it might be necessary to explicitly drop a StopIf after it polls as completed. This is not the case for Flatten, which will drop the underlying signal automatically once it polls as completed.

Because these behaviors are different, they could easily trip up users trying to implement custom signal adapters, especially if any of the signals involved have custom drop logic. One might expect from eg. StopIf, FilterMap, that underlying signals are dropped once the adapter is dropped; This would not be a valid assumption for Flatten, which drops the inner and outer signals respectively as soon as they poll as complete. Likewise a user expecting signals to be dropped by adapters once polled as completed would be surprised by StopIf and FilterMap.

In any case, the different drop behaviors should be explicitly documented. There are some signal adapters for which I am still uncertain of drop behavior due to implementation via macros, eg. map_ref.

[Pauan]

When writing a signal adapter that owns StopIf, it might be necessary to explicitly drop a StopIf after it polls as completed.

That is the responsibility of the combinator method.

The Future/Signal system guarantees that everything will get dropped properly, so if it's not dropping properly then that's a bug that needs to be fixed.

Could you explain more about your use case and how you're using stop_if? And why the existing drop behavior of stop_if does not work for you?

One might expect from eg. StopIf, FilterMap, that underlying signals are dropped once the adapter is dropped

I'm not sure what you mean by "adapter", but every Signal is dropped as soon as the downstream consumer is dropped (which can happen explicitly, or implicitly when the entire task is dropped).

In general it's very very very hard to accidentally leak memory, and you don't need to worry about it: the memory is managed automatically.

If you are writing your own Signal combinator, then yes you need to be aware of various internal details (including the Signal contract). You need to understand how the methods combine in a zero-cost way (with struct inlining). You need to understand how Future tasks are allocated and deallocated, and it's very helpful to understand how Future executors work internally.

Creating your own Signal combinators is very much so expert-only, it's not something that regular users should need to worry about: you can just call the existing combinator methods and everything should work correctly.