Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change share(replay:scope:)'s default scope to .forever #1527

Closed
wants to merge 1 commit into from

Conversation

inamiy
Copy link
Contributor

@inamiy inamiy commented Dec 14, 2017

Hi @kzaher ,

There is an issue in share(replay:scope:) that uses .whileConnected as a default value.

When calling .share() without any arguments, this will no longer behave as previous .publish().refCount() in v3.6.1, but will work quite differently as follows, especially when upstream is finite cold observable:

let o = Observable.of(1, 2) // finite cold
    .map { x -> Int in
        print("[map]", x)
        return x
    }
    .share()

print("--- 1 ---")
o.subscribe(onNext: { print("[sink1]", $0) })
print("--- 2 ---")
o.subscribe(onNext: { print("[sink2]", $0) })
--- 1 ---
[map] 1
[sink1] 1
[map] 2
[sink1] 2
--- 2 ---
[map] 1
[sink2] 1
[map] 2
[sink2] 2

Replacing above with .share(scope: .forever) will work as expected (same as .share() in v3.6.1):

--- 1 ---
[map] 1
[sink1] 1
[map] 2
[sink1] 2
--- 2 ---
(no output for 2nd subscription because 2nd connection to cold observable will not be produced)

Since .share() as .publish().refCount() is the right implementation for ReactiveX, I think it is better to have .forever as a default scope instead.

@kzaher
Copy link
Member

kzaher commented Dec 14, 2017

Hi @inamiy ,

This is an official change, please take a look at CHANGELOG.md (4.0.0-beta.1):

Deprecates shareReplayLatestWhileConnected and shareReplay in favor of share(replay:scope:). #1430
Changes publish, replay, replayAll to clear state in case of sequence termination to be more consistent with other Rx implementations and enable retries. #1430
Replaces share with default implementation of share(replay:scope:). #1430

There were also PRs that announced those changes.

Share replay only makes sense when one has non terminating observable sequence and its main use case is optimization.

e.g.:

  • sharing database observers (coredata, firebase, ....)
  • sharing websocket connection
  • used inside of Driver and Signal

Another motivation is that share(replay:scope: .whileConnected) works with replay operators where share(replay:scope:.forever) doesn't.

The reason why we've had .forever as a default strategy is because we've tried to be consistent with original Rx.NET, RxJs versions, but later realized that newer versions of RxJava and RxJs both used .whileConnected.

I was thrilled to find that out because I've considered that original .forever strategy to be an unfortunate design flaw in Rx.NET that we were living with.

There are multiple problems of using share(replay:) with .forever in most common use cases:

  • one can't clear internal cache on dispose (replay operators won't work)
  • if value is cached and there is no expiration date, if you subscribe to an observable after significant time in, you'll unexpectedly initially get stale value from a long time ago and potentially some error that was also cached long time ago.

I would say that there might be a couple of edge use cases that might justify its usage, but these are really edge cases IMHO.

Using any type of sharing operator with a predictably finite sequence is a code smell IMHO.

@inamiy
Copy link
Contributor Author

inamiy commented Dec 14, 2017

@kzaher
Thanks for detailed explanation :)
Yes, I agree with most of your mentions, except in particular to publish().refCount(), it doesn't have internal cache to replay, so .whileConnected's replayable feature is often unnecessary.

I personally think share() sticking into publish().refCount() is simpler than thinking of ShareWhileConnected that may replay after certain scenarios (but maybe it's too edge case that most of the users don't care or mistake).

The reason why we've had .forever as a default strategy is because we've tried to be consistent with original Rx.NET, RxJs versions, but later realized that newer versions of RxJava and RxJs both used .whileConnected.

I wasn't aware of this. Do you have a reference?
I could only find publish().refCount() version in RxJava (raw source code)...
(Note: 10k LOC couldn't display all in GitHub viewer!)

@kzaher
Copy link
Member

kzaher commented Dec 14, 2017

Yes, I agree with most of your mentions, except in particular to publish().refCount(), it doesn't have internal cache to replay, so .whileConnected's replayable feature is often unnecessary.

You can't use share()/publish().refCount() for the same scenarios you use share(replay: 1). They have completely different semantics. One models shared event hub, other one models state.

I personally think share() sticking into publish().refCount() is simpler than thinking of ShareWhileConnected that may replay after certain scenarios (but maybe it's too edge case that most of the users don't care or mistake).

share(replay: 1)/Driver is by far the most common use case in my scenarios. If you are using state/database observers, use cases are pretty straightforward.

I wasn't aware of this. Do you have a reference?
I could only find publish().refCount() version in RxJava (raw source code)...
(Note: 10k LOC couldn't display all in GitHub viewer!)

You can check out source code and create some test examples.

@inamiy
Copy link
Contributor Author

inamiy commented Dec 15, 2017

Yes, I know share(replay: n) where n > 0 fits well with .whileConnected in many scenarios, but I'm only curious about whether .whileConnected is necessary for case n = 0, when simply calling share().

I'm not still sure about other ReactiveX implementation, but if share() = publish().refCount() still has a consensus, I think distinguishing share() and share(replay:) is meaningful.

One possible approach would be:

public func share() {
    return share(replay: 0, scope: .forever) // same as `publish().refCount()`
}

public func share(replay: Int /* no default value */, scope: SubjectLifetimeScope = .whileConnected) {
    ...
}

Or, if we don't really need to care about other platforms too much, keeping the current implementation is good enough.

@inamiy
Copy link
Contributor Author

inamiy commented Dec 16, 2017

Ref:

RxJS doesn't seem to have .whileConnected feature too.

@kzaher
Copy link
Member

kzaher commented Dec 16, 2017

Yes, I know share(replay: n) where n > 0 fits well with .whileConnected in many scenarios, but I'm only curious about whether .whileConnected is necessary for case n = 0, when simply calling share().

It doesn't matter if n = 0 because error caching is also important. If operator caches error forever, retry operator won't work as expected, and yes, this is extremely important property. e.g. Websockets.

I'm not still sure about other ReactiveX implementation, but if share() = publish().refCount() still has a consensus, I think distinguishing share() and share(replay:) is meaningful.

I'm not sure what do you mean by this. We are consistent with all of them. They have exactly the same behavior when you don't explicitly specify sharing strategy and just write share() (except for .NET which is not consistent with the rest, but you can also get that behavior).

Or, if we don't really need to care about other platforms too much, keeping the current implementation is good enough.

There are different Rx implementations out there that have different behavior regarding this. Two major groups of behaviors are:

  • whileConnected
  • forever

Rx.NET -> forever
Rx.JS -> previously forever, now whileConnected
RxJava -> whileConnected

I think that whileConnected is sensible default value, but user always has a choice which behavior to adopt.

Ref:
RxJS.share
RxJS.shareReplay

RxJS doesn't seem to have .whileConnected feature too.

If you are implying that it doesn't have that enum in public interface, that is correct. It only supports .whileConnected behavior and javascript doesn't even have a concept of enums (although you can model them if you like).

Or did you refer to something else?

@inamiy
Copy link
Contributor Author

inamiy commented Dec 16, 2017

It doesn't matter if n = 0 because error caching is also important. If operator caches error forever, retry operator won't work as expected, and yes, this is extremely important property. e.g. Websockets.

So, the main goal here is really "retry" availability, which means "share while connected and retryable after all disconnected or received error/completed", right?
And we just call it "share()"... 🙄
This sounds beyond the simple idea of "share means hot", since retry normally has cold semantics.

But you are definitely right that ReactiveX itself now has shareReplay with .whileConnected as default feature as you mentioned.
At least I now realized that RxJS.shareReplay is re-creating ReplaySubject and re-subscribing to source again when it previously emitted error or completed, so that's .whileConnected behavior.

So anyway...

Since you've already digged into other platform and confirmed the new behavior, I'm now 👍 to .whileConnected as new default value.

And I think #1527 (comment) will be useful info to attach to the documentation.

I can now close this PR. Thanks for discussion, @kzaher :)

@inamiy inamiy closed this Dec 16, 2017
@inamiy inamiy deleted the fix-share branch December 16, 2017 17:57
@kzaher
Copy link
Member

kzaher commented Dec 16, 2017

So, the main goal here is really "retry" availability, which means "share while connected and retryable after all disconnected or received error/completed", right?

That's one part of problem that is solves. The other one is stale cache of sequence elements.

This sounds beyond the simple idea of "share means hot", since retry normally has cold semantics.

IMHO hot and cold is simply a fairy tale that I often see being mentioned on twitter in cases when:

  • somebody either doesn't have time to explain how rx really works
  • they think it will be simpler to explain some concepts to new users by speaking less precisely
  • they are speaking in some specific context where they want to describe how big are subscriptions costs
  • for promotional reasons, such as, "we've solved hot/cold issues that those other guys have", come to us
  • one doesn't understand how rx really works
  • some other category

If somebody asks you what about hot and cold, try to perform this experiment.

Ask them, "Is web socket hot or cold"?

  • answer = cold -> this is where you want them to be
  • answer = hot -> then you ask, "and how about url request":
    • answer = hot -> then you ask them for some examples of cold and assume they are high or something
    • answer = cold -> then ask them, "we'll isn't web socket in essence just never ending http connection, what is the difference?"

First milestone should be easily achieved.

let event = PublishSubject<()>() // or equivalent in some other library
let result = event.map { "Here comes new event" }
let result2 = event.map { "Here comes new event" }.share()

Then ask them: "Is event hot or cold?":

  • if they say "hot" -> this is where you want them to be
  • If they say "cold" -> assume they are high

Then ask them "is result hot or cold":

  • answer = "hot" -> this is where you want them to be
  • answer = "cold" -> please let me know the reason, because I'm also curious how one will explain this. Then ask the followup question, "Is result2 hot or cold?"
    • answer = cold -> please let me know the reason, because I'm curious how one will explain this
    • answer = hot -> this is where you want them to be.

So here we are:

  • we should be able to persuade someone that web socket is cold
  • we should be able to persuade someone that
let result = event.map { "Here comes new event" }
let result2 = event.map { "Here comes new event" }.share()

at least one of result and result2 is hot. They'll probably say that both are.

And now the final part.

Well isn't websocket equivalent to:

let event = PublishSubject<()>() // this is a publish subject on server
let result2 = event.map { "Here comes new event" }.share() // this is result on client

How can websocket API be both hot and cold, isn't this a contradiction?

And I think #1527 (comment) will be useful info to attach to the documentation.

I'm assuming Google will index it somehow but yes, it would be nice to improve the docs. We are open for PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants