Double-sent requests #238

Closed
staltz opened this Issue Feb 25, 2016 · 29 comments

Projects

None yet

7 participants

@staltz
Member
staltz commented Feb 25, 2016

From @staltz on July 17, 2015 9:2

function main(responses) {
  console.log('This only gets called once.')
  const url = '/ontologies/acl.ttl';
  var request$ = Cycle.Rx.Observable.just(url);

  var vtree$ = responses.http
    .do(r => console.log('this gets called twice', r))
    .mergeAll()
    .do(r => console.log('this gets called once', r))
    .map(res => h('p', res.text.substr(0, 100) + '...'));

  return {
    dom: vtree$,
    http: request$
  };
}

let [requests, responses] = Cycle.run(main, {
  dom: makeDOMDriver('#app-container'),
  http: makeHTTPDriver(),
  db: makeLocalDbDriver(),
});

Copied from original issue: cyclejs/cycle-http-driver#9

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 11:27

For me it looks like the request is sent 5 times :-)

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 11:27

I mean, I am sending a single GET request in my code. I am not running this particular example...

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 11:56

Actually not sure mine is the same case. I am using mergeAll and I am seeing everything once on the client, but the server is receiving the same request 5 times...

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 12:2

Yeah, seeing 5 responses before mergeAll...

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 14:9

Was trying to find the issue. Not sure why, but while createResponse$ is called exactly once, the callback in Rx.Observable.create in the same function is called 5 times.

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 14:17

I mean, the create function itself is called 5 times. Totally weird...

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 17, 2016 19:55

Patched this lib to wrap superagent in a promise and then used Rx.Observable.fromPromise and it started working properly...

@staltz
Member
staltz commented Feb 25, 2016

From @arnodenuijl on January 22, 2016 20:39

I think I had the same problem. One request going in to the driver and two requests being made to the server. laszlokorte explained on gitter that the function function createResponse$(reqOptions) in the driver creates a cold observable. So every subscription to the response$ results in an extra call being made. Work around is to call share() on the response$ you get from the driver to make the observable hot or to make the request eager which has the 'side' effect to make the response$ hot.

If you had five requests going out then it looks like you had five effective subscriptions. I think wrapping the call in a promise has the same work around effect (making the response$ hot).

I would expect the response$ to always be hot. Can't think of a situation where this 'cold' behavior is useful. (but maybe someone else can?!).

See http://arnodenuijl.github.io/CycleDoubleHttp/index.html for a working example where I had two calls being made off of one request. Just open the network tab in the developer tools and click the button. You will see two requests being made for every click. The source code is on https://github.com/arnodenuijl/CycleDoubleHttp/blob/master/src/app.ts. In my case I used the HTTP response$ for generating the state$ (line 35). And the state$ was used for generating the DOM (line 42) and for creating the HTTP request$ (line 69).

By using the state$ in two places, one going into the DOM driver and one going into the HTTP driver, two subscriptions are effective and two requests are being made.

@staltz
Member
staltz commented Feb 25, 2016

From @tchap on January 23, 2016 14:17

@arnodenuijl Cheers, will take a look when I have time.

@staltz
Member
staltz commented Feb 25, 2016

From @laszlokorte on January 23, 2016 14:36

@arnodenuijl Now that I think about it the reason for the observable being cold is pretty clear: If the request fails, a cold observable can retry the request just by subscribing again. With a hot observable you can not retry the request (without creating a whole new one which might not be possible if you do not know all the parameters or do not have access to the request sink or if the one finally consuming the response does not know about cycle but just consumes failable observables)

@staltz
Member
staltz commented Feb 25, 2016

I would expect the response$ to always be hot. Can't think of a situation where this 'cold' behavior is useful. (but maybe someone else can?!).

@arnodenuijl Like everything else with RxJS, cold is the default. I actually agree with you that I expect hot as default, but this is just how RxJS is, always cold by default. And there are deep implications for that. Cold is more general than Hot, so there's a reason why it's default. It's useful specially in cases where you need to retry, like Laszlo said.

So rather, than going against the RxJS normal behavior, I would stick to keeping the cold behavior, and instructing people to .share() those. Cycle may soon get support for multiple stream libraries, and then those may allow a hot-by-default approach.

@vladap
vladap commented Mar 7, 2016

I did some small POC with Cycle some time ago. I remember I had some problem using .share on responses from HTTPDriver and had to use .shareReplay(1) instead. If you would kill me I can't remember how the problem has manifested, so I apologize it is really low value input. Because I'm still Rx noob my naive reasoning at the time was something along the lines that HTTP driver is using Subject somewhere in the way hence it has to use .shareReplay(1) instead of .share()... which might be completely wrong reasoning.

Anyway it is more likely that the fault was on my side because I was using decorator for HTTP Driver to provide some default behaviour for HTTP errors, mainly unauthorized and I was sharing by default. Is there something wrong with my code which could cause some problems down the line?

I'm just pretty sure there was something wrong with using .share() in the code below. I will try to replicate if you won't see a problem. Just I'm finishing a sprint at the moment and still behind, so little time right now.

export function createHttpMiddleware(httpDriver, history) {
  return req$ => {
    const handleUnauthorized = err => {
      if (err.response && err.response.statusCode === 401) {
        history.push({pathname: '/login'})
      }
      return Observable.just(err)
    }

    return httpDriver(req$)
      .map(x => {
        let res$ = x.catch(handleUnauthorized)
        // catch nests request one level lower
        if (res$.source.request) {
          res$.request = res$.source.request
        }
        return res$
      })
      .shareReplay(1)
  }
}

EDIT: I think the problem was that component listening to HTTP responses was not rendered at all.

@vladap
vladap commented Mar 7, 2016

Note, that the pattern is trying to achieve... when you are already logged on some page and your auth expires and you e.g. click a button which generates protected http request you are redirected to login form. It is missing returning back original routed when successfully logged in again. And the pattern is possibly not the best way how to do it in the first place.

@vladap
vladap commented Mar 7, 2016

I went through my commit messages in that POC and this one problem was fixed by replacing .share() by .shareReplay(1):

"Table component didn't triggered HTTP request, not receiving data resulting in an empty page."

The code for Table request$ was standard one, just create request object and return it to decorated HttpDriver.

@felipecsl

It still makes double requests for me, tried both share() and shareReplay(1)

@laszlokorte
Contributor

@felipecsl could you show your code?

@ivan-kleshnin
Contributor

I did some small POC with Cycle some time ago. I remember I had some problem using .share on responses from HTTPDriver and had to use .shareReplay(1) instead. If you would kill me I can't remember how the problem has manifested, so I apologize it is really low value input.

@vladap shareReplay(1) is "required" for streams with scan() otherwise they don't broadcast a "current" state on subscribe. Usually it leads to initial state being "missed". So my rule of thumb is to add share() to every stream you expose (outside function / module) and to add shareReplay(1) to every stateful stream.

The other thing with scan() is that such streams are moving away from event streams to data streams. For event streams timing is crucial so --1--1--> may very well differ from --1---->.

For data streams they are kinda the same so we can discard adjacent events carrying the same data. Hence avoiding excessive recalculations bound to a state change. So you'll probably want to distinctUntilChanged() streams with scan() most of the time.

I'm actually playing with a store abstraction:

// always :: a -> b -> a
let always = curry((x, y) => x); // constant function

// scanFn :: s -> (s -> s) -> s
let scanFn = curry((state, updateFn) => {
  if (typeof updateFn != "function" || updateFn.length != 1) {
    throw Error("updateFn must be a function of arity 1")
  } else {
    return updateFn(state);
  }
});

// store :: s -> Observable (s -> s)
let store = curry((seed, update$) => {
  return update$
    .startWith(seed)
    .scan(scanFn) 
    .shareReplay(1)
    .distinctUntilChanged();
});

// do not use `scan(seed, scanFn)` option because you *need* to cast seed state
// and RxJS does that only with `startWith()` (unlike other FRP libs)

Which works great with currying.
You just need to feed a "store" unary functions (from state to state or s -> s).

let update = { // a bucket of update channels
  users$: new Subject(),
};

let state = { // a bucket of state reducers
  users$: store([], update.users$), 
}; 

// RxJS v5
update.users$.next(append({id: "1", name: "John Doe"})); // push first user
update.users$.next(append({id: "2", name: "Jane Doe"})); // push second user
update.users$.next(always({}));                          // reset users
@Cmdv
Contributor
Cmdv commented Mar 15, 2016

@staltz we still classing this as a bug? should I implement things to be hot in the API or update docs to show the use of share() / shareReplay(1)

@felipecsl

for me, using eager: true fixed this, as stated on #262

@Cmdv
Contributor
Cmdv commented Mar 15, 2016

@felipecsl ah ok cool, well maybe updating docs + extra tests will solve this issue. I'll add more info to the docs like that there is no confusion and create a test for good measures

@staltz
Member
staltz commented Mar 15, 2016

lets close this issue, but the plan is to make eager be the default. Actually all sources should be multicast observables (hot).

@staltz staltz closed this Mar 15, 2016
@Cmdv
Contributor
Cmdv commented Mar 15, 2016

ok cool, well if we need that in the api create new issue and I'll pick it up 😄

@laszlokorte
Contributor

@ivan-kleshnin That's a really good summary and explanation of the usage of scan, share and shareReplay! I would love to see that published somewhere in the cycle or rx docs.

@ivan-kleshnin
Contributor

@laszlokorte thanx!

This is far from a summary but I'm going to extend this info and publish an article on my site.
So RxJS guys may link it somewhere on their "additional readings" page if they find it useful.
I will leave a link here as well if nobody's against.

@vladap
vladap commented Mar 16, 2016

This share() here and .shareReplay(1) there is quite confusing. I probably understand now one case when to use what but still I don't understand why and what is the technical difference between them. Which leads me to doubts if there are other edge cases. It looks a bit leaky, like I have to know how streams works underneath.

I basically understand this hot/cold thing as caching/memoizing some part of computation. In Apache Spark I call .cache() on its DAG to avoid recomputations, Apache Flink can even detect in its optimizer when it needs to should use it (at least some cases, I haven't study its details yet).

Has been share() and shareReplay(1) somehow reworked in RxJs 5 for better good or is it quite same?

@ivan-kleshnin
Contributor

I apologize for spamming this thread 😥 Here is one final post.

Has been share() and shareReplay(1) somehow reworked in RxJs 5 for better good or is it quite same?

It's getting better.
For example, they renamed replay to publishReplay which is a good clarification.

I'll try to explain. Everything is quite simple once you get it 😄


  1. RxJS has cold Observable and hot Subject primitives. In short: cold is deterministic streams while hot is for non-deterministic ones. That's a long story so I'll just say I would prefer having Hot by default. Requirement of manual share() is not satisfying and error-prone.

  2. To make an Observable behave (like) hot RxJS fake it using Subject under the cover.
    Because this is the simplest implementation.

  3. RxJS exposes a multicast() operator which does what desribed in 2.
    But it expects an exact Subject type to work. And there is more than one...

  4. So RxJS predefines two flavors of multicast. Just for your convenience.

    publish()       // multicast() with Subject() (regular subject)
    publishReplay() // multicast() with ReplaySubject() (replay subject)

    If you look at the RxJS sources – those above are almost oneliners (extra code is for polymorphism and other stuff irrelevant for explanation).

  5. There is one more step. Hot Observables need to be run manually (almost by definition).
    So you call connect() on them to start event flow.

    But often you want to avoid this manual book-keeping and fire a flow
    automatically once you have a first subscriber.

    So RxJs implements a refCount() operator which does that for you.

    These are also oneliners. One for each code branch above.

    share()       // publish() with refCount()
    shareReplay() // publishReplay() with refCount()

To recap

// Generic Cold -> Hot
.multicast() // requires Subject type and connect()

// Basic Cold -> Hot 
.publish() // multicast() + regular Subject(), requires connect()

// Basic Cold -> Hot with auto-connect
.share() // publish() + refCount()

// Replaying Cold -> Hot
.publishReplay() // multicast() + ReplaySubject(), requires connect()

// Replaying Cold -> Hot with auto-connect
.shareReplay() // publishReplay() + refCount()

Of course all above is oversimplified but I hope you've got the point 😉
My explanation still misses two points:

  1. Why Hot is Multicast and Cold is Unicast? Or vice versa...
  2. What is actually "replaying"?

But (as I said), I'm going to write an article and I don't have a space to cover all this here.

@TylorS
Member
TylorS commented Mar 16, 2016

@ivan-kleshnin I'm probably just being nit-picky but there is also publishBehavior() which is equal to multicast() + BehaviorSubject(). Also if you're trying to explain specifically RxJS 5 (?); shareReplay() is no longer an operator as far as I can tell.

Thanks for the nice write up, and look forward to your article.

@ivan-kleshnin
Contributor

I'm probably just being nit-picky but there is also publishBehavior() which is equal to multicast() + BehaviorSubject()

Yeah. I just never used that one.

Also if you're trying to explain specifically RxJS 5 (?); shareReplay() is no longer an operator as far as I can tell. Thanks for the nice write up, and look forward to your article.

No I wasn't trying to be RxJS 5 specific.

Thank you too. I'll try to do my best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment