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

events from fromEvent + scan out of order #491

Open
Risto-Stevcev opened this issue Sep 17, 2017 · 16 comments
Open

events from fromEvent + scan out of order #491

Risto-Stevcev opened this issue Sep 17, 2017 · 16 comments

Comments

@Risto-Stevcev
Copy link

I have the following code:

var createStore = function(name, reducer, initialState) {
  var storeName = 'store:'+ name

  var stream = most.fromEvent(storeName, document)
    .scan(function(state, event) {
      return reducer(state, event.detail)
    }, initialState)

  var subscribe = function(callback) {
    stream.subscribe({
      next: callback
    })
  }

  var dispatch = function(action) {
    document.dispatchEvent(new CustomEvent(storeName, { detail: action }))
  }

  return {
    stream: stream,
    subscribe: subscribe,
    dispatch: dispatch
  }
}


var store = createStore('foo', function(state, action) {
  switch (action.type) {
    case 'SET_FOOBAR':
      return { foobar: state.foobar + action.foobar }
    default:
      return state
  }
}, { foobar: 1 })

store.subscribe(console.log)
store.dispatch({ type: 'SET_FOOBAR', foobar: 3 })
store.dispatch({ type: 'SET_FOOBAR', foobar: 5 })

which strangely enough prints the initial state as the last thing to the console:

Object {foobar: 4}
Object {foobar: 9}
Object {foobar: 1}

I would expect it to print the inital state as the first thing, like this:

Object {foobar: 1}
Object {foobar: 4}
Object {foobar: 9}

is this a bug?
I'm trying this on 1.7.0. I've tried this code with rxjs and xstream and they both work like I expected it to

@Risto-Stevcev
Copy link
Author

Risto-Stevcev commented Sep 28, 2017

I found out that if I move it inside a setTimeout then it works like I would expect it to:

setTimeout(function() {
  store.dispatch({ type: 'SET_FOOBAR', foobar: 3 })
  store.dispatch({ type: 'SET_FOOBAR', foobar: 5 })
}, 0)

I guess there's something going on with how it queues up events in the event loop

@zkochan
Copy link

zkochan commented Dec 27, 2017

I have experienced this issue when using most.from with an observable created by zen-push. Moving pushStream.next('foo'); into a setTimeout() fixed the issue in my case as well.

@davidchase
Copy link
Collaborator

@Risto-Stevcev document.dispatchEvent dispatches synchronously (mdn: the event handlers run on a nested callstack: they block the caller until they complete)

so that's why you get the unexpected result until you wrap your dispatching into a setTimeout.

@zkochan
Copy link

zkochan commented Dec 28, 2017

Would be better to throw some error in cases like this. It is a silent error and really hard to track down.

@Frikki
Copy link
Collaborator

Frikki commented Dec 28, 2017

It is a silent error ...

It is not an error. In fact, it behaves as expected. It would be wrong to “throw some error in cases like this.”

Within a function f, a failure is an error if and only if it prevents f from meeting any of its callee’s preconditions, achieving any of f’s own postconditions, or reestablishing any invariant that f shares responsibility for maintaining.

There are three different kinds of errors:

  • a condition that prevents the function from meeting a precondition (e.g., a parameter restriction) of another function that must be called;
  • a condition that prevents the function from establishing one of its own postconditions (e.g., producing a valid return value is a postcondition); and
  • a condition that prevents the function from re-establishing an invariant that it is responsible for maintaining. This is a special kind of postcondition that applies particularly to member functions. An essential postcondition of every non-private member function is that it must re-establish its class’s invariants.

Any other condition is not an error and should not be reported as an error.

@Risto-Stevcev
Copy link
Author

There's something odd about this behavior though. I've tried doing similar things with fromEvent equivalents from xstream and rxjs and they don't have this issue.

@TylorS
Copy link
Collaborator

TylorS commented Dec 28, 2017

Most basically uses setTimeout or Promise.resolve(task).then(runTask) to schedule the default value from scan at the time of subscription. Which gives exactly one event loop to synchronously call dispatchEvent and emit before the default value has the chance to emit.

Whereas other libraries will emit the default scan value at the time of subscription - ensuring it occurs before any others could possibly make it through.

This may seem odd at first, but this guarantee that events do not occur in the same call stack as subscription makes your applications less prone to errors that occur due to the order in which you subscribe to your streams.

@zkochan
Copy link

zkochan commented Dec 28, 2017

I still think this is very confusing. Maybe it is obvious to you but for such a beginner in FRP like myself, it was really hard. I didn't believe my eyes when I've seen that the initial value happened first.

It should be at least mentioned in the API docs, with bold font and examples.

@Risto-Stevcev
Copy link
Author

Risto-Stevcev commented Dec 28, 2017

@TylorS

This may seem odd at first, but this guarantee that events do not occur in the same call stack as subscription makes your applications less prone to errors that occur due to the order in which you subscribe to your streams.

Can you be more specific (maybe an example) on how this prevents errors? Emitting the default scan value at subscription seems like the correct design decision to me

@Risto-Stevcev
Copy link
Author

@Frikki Not sure I would call it an error either, but scan definitely behaves differently in this particular circumstance than any other circumstance that it's used, so it's not conforming to the API spec

@axefrog
Copy link
Contributor

axefrog commented Dec 28, 2017

I can't see exactly why this is happening in this case, but I've experienced the initial scan value arriving out of order when the time value of the initial inbound event is generated before the scan source is run. The scheduler schedules the initial value based on the current time, and if the inbound value is delivered to the scan sink in the same call stack, you end up with two scheduled tasks, the second having an earlier schedule time than the first, which causes the scheduler to arrange for the second to be delivered before the first.

@Frikki
Copy link
Collaborator

Frikki commented Dec 29, 2017

As I see it, @axefrog, the culprit is what @davidchase wrote here, and is aligning with your comment, too.

@Risto-Stevcev, I still think that most.scan() does what it should, but the dispatchEvent() call blocks the caller and messes up the order:

... the event handlers run on a nested callstack: they block the caller until they complete ...

If we, as @TylorS mentions in his comment, didn’t ensure that events occur on a different callstack than the subscription, we would see race condition issues in many cases.

This relates to this issue, and the solution chosen was to develop @most/core and get rid of these not-really-true-event-streams (see @TylorS’s and @briancavalier’s comments towards the end of the issue).

@Risto-Stevcev
Copy link
Author

Is there a downside to modifying the implementation of scan so that it applies setTimeout(..., 0) to the initial value of a scan if it's coming from an event?

That way none of this would occur and would ensure that all events occur on a different callstack than the subscription

@ryanlaws
Copy link

ryanlaws commented Mar 1, 2018

@Risto-Stevcev, in my testing,setTimeout(fn, 0) delays the call to fn by up to 12 milliseconds (usually about 5ms), even in the absence of anything else in the window, so that implementation isn't safe for all purposes.

@Risto-Stevcev
Copy link
Author

@ryanlaws DOMHighResTimeStamp is in ms not mcs, could also be that you included the function declaration by accident:

const f = () => 123
const timeout = () => { performance.mark('fn-start'); setTimeout(f, 0); performance.mark('fn-end') }
timeout()
performance.measure('fn', 'fn-start', 'fn-end')
performance.getEntriesByName('fn')[0].duration
// vs
timeout2 = () => { performance.mark('fn2-start'); f(); performance.mark('fn2-end') } 
timeout2()
performance.measure('fn2', 'fn2-start', 'fn2-end')
performance.getEntriesByName('fn2')[0].duration

On my machine the difference is about 0.06-0.08 ms, which is 60-80 microseconds. That's very negligible

@ryanlaws
Copy link

ryanlaws commented Mar 1, 2018

@Risto-Stevcev Thanks for the test snippet. It's showing a difference of just shy of .1ms on my machine. It's very possible that my test was flawed - I believe I was directly calling performance.now() and subtracting instead of performance.mark() so the former may account for the extra milliseconds. I ran it months ago and I don't have my test code handy, so I trust yours over mine for sure.

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

No branches or pull requests

7 participants