staltz / xstream

An extremely intuitive, small, and fast functional reactive stream library for JavaScript
http://staltz.github.io/xstream/
MIT License
2.37k stars 137 forks source link

MemoryStream should not forget its last value on complete #310

Open CarlLeth opened 4 years ago

CarlLeth commented 4 years ago

I believe this is the most surprising and inconsistent behavior in xstream. The second bullet in the introduction to xstream says "Only "hot" streams", but subscribing to a closed MemoryStream can cause a deep replay of past events. That seems like distinctly "cold" behavior, and it has consequences. It means that the behavior of your program can completely change based on internal state that's outside of the abstraction of Stream.

Scanning through the issues list, I see this over and over. Here's a quick list of issues either fully or partly caused by this:

The reason I choose xstream over other libraries is exactly that second bullet: only hot streams. That means I can think in only one direction: forward, and I know that later streams and listeners can't cause side-effects that back-propagate throughout my stream graph. But that's exactly what's happening here: the behavior of my streams can change based on what's happening ten modules downstream.

What's the other side of the argument? Why would you want to keep this behavior?

I've seen the discussion at https://github.com/ReactiveX/rxjs/issues/453, but it's filled with talk of ReplaySubjects and re-connectable observables and exactly the sort of thing I thought "only hot streams" meant we don't have to bother with.

wclr commented 4 years ago

This also raises the question, should stream (actually not only with memory) die if it has no more subscribers? If to follow your logic stream should be always alive and ready to be re-connected. Does hot in your view means always alive or always ready to raise from the dead and continues to emit the way it had started in the beginning?

matthewjamesadam commented 2 years ago

I have run into this as well. In my case I have helper methods that run expensive async code in the middle of a stream, using an approach similar to https://github.com/staltz/xstream/issues/2:

someStream
    .map(s => xstream.fromPromise(SomeExpensiveAsyncFn))
    .flatten()
    .remember()

Every time I unsubscribe and resubscribe to someStream, SomeExpensiveAsyncFn is called. I would have expected .remember() to cache this value so it wouldn't be necessary. This makes the async pattern far less useful, unfortunately, and the only way I can think of to fix this is to build a state machine around the stream to hold the last value, which isn't great.

staltz commented 2 years ago

The reason I haven't responded to this issue yet is because it's not obvious whether MemoryStreams should forget or not forget. There are use cases where you need the MemoryStream to never forget (this issue), and there are use cases where you need to do cleanup and "reset", otherwise you could create memory leaks by retaining those forever.

In your case, @matthewjamesadam, I think you can opt-in to never resetting the stream by forcing the stream to never complete.

 someStream
+    .compose(dropCompletion)
     .map(s => xstream.fromPromise(SomeExpensiveAsyncFn))
     .flatten()
     .remember()

+function dropCompletion(stream) {
+  return xs.merge(stream, xs.never())
+}
matthewjamesadam commented 2 years ago

Thanks @staltz -- I can definitely appreciate that there is no single "right" behaviour here. Perhaps a parameter on .remember() could specify which behaviour should be used on the stream?

Thanks for the suggestion -- unfortunately for some reason it doesn't seem to be working for me. My scenario is ultimately pretty complicated so something else in my stream might be causing this problem.

CarlLeth commented 2 years ago

there are use cases where you need to do cleanup and "reset", otherwise you could create memory leaks by retaining those forever.

Would it retain the object forever, or just until the MemoryStream itself gets GCed? If you call .remember() on a stream that emits some expensive object, I think that stream possibly taking up as much memory as that expensive object is exactly what you'd expect. It's not surprising that a Promise of an Expensive Object would itself be expensive (once the promise completes) until that promise is disposed of. I use .remember() when I want something "promise-like": a state-holder whose state might not be available yet, with the added benefit that I can know when it changes.

Perhaps I'm using Streams differently than you expected, but I don't really encounter cases where closed streams are sticking around in memory forever.