cujojs / most

Ultra-high performance reactive programming
MIT License
3.5k stars 231 forks source link

Why does zipping with periodic() hangs? #466

Closed iwinux closed 6 years ago

iwinux commented 7 years ago

Summary

I want to print a stream of increasing numbers every 1s, but the code below hangs Node.js forever:

// test.js
const { iterate, periodic } = require('most')

iterate(x => x + 1, 0)
  .take(10)
  .zip(x => x, periodic(1000))
  .forEach(x => console.log(x))

while this

iterate(x => x + 1, 0)
  .take(10)
  .forEach(x => console.log(x))

terminates as expected.

Expected result

Expected output would be:

$ node test.js
0
1 # after 1s
2 # after 2s
...

Actual Result

$ node test.js
0 # <- hangs here and CPU usage goes up

Versions

briancavalier commented 7 years ago

Hi @iwinux, thanks for reporting this. The sample code to reproduce was very helpful! It appears to be a regression in take introduced in #428 (technically, a regression in slice, on which take is based). The regression is causing iterate to run forever, and continuously preempt the periodic task.

The reason it manifests with zip is that zip has to buffer the very fast iterate events, while waiting for corresponding periodic events, which never occur due to the preemption mentioned above.

The fix is relatively simple, and I should have time tonight to create a branch so you can try it.

If you're indeed blocked by needing a periodic counter, then this will work and hopefully unblock you until the fix is ready:

periodic(1000)
  .slice(1, 10)
  .scan((x, _) => x + 1, 0)
  .forEach(x => console.log(x))

I'm guessing your example is a simplified version of a real use case, so this may not help, but figured it was worth a shot. Cheers!

iwinux commented 6 years ago

Thanks! Glad to see that root cause is found 👍

briancavalier commented 6 years ago

@iwinux There's a candidate fix in #467 (fix-466 branch). Could you give it a try and let us know if it works for you?

UPDATE: There's a slightly cleaner fix in #468 (fix-466-2 branch). Please give that one a try and let us know. Thanks!

iwinux commented 6 years ago
$ git clone https://github.com/cujojs/most.git
$ git checkout fix-466-2
$ npm install # some build error coming from contextify
$ npm run build

This version seems to fix it. The test case no longer hangs. Thanks!

BTW: should iterate always be combined with take or slice? How to create a lazy & infinite stream that emits one value every second?

briancavalier commented 6 years ago

This version seems to fix it.

:+1: Thanks for trying it. We'll get this merged and release a patch version soon.

should iterate always be combined with take or slice?

It depends on your use case. Usually, yes, because iterate, by itself, is always infinite, as are unfold and periodic, and any stream created by applying from to any existing infinite source, such as an infinite iterable.

However, the step function you provide to iterate may return a promise--i.e. instead of returning an number, it can return Promise<number>. Thus, the promise can be used to control the timing.

How to create a lazy & infinite stream that emits one value every second?

It depends on what "value" you want the events to have. Usually, periodic is the right place to start. For example, if you want an infinite stream containing 1 every second:

periodic(1000).constant(1)

If you want an infinite stream that starts at 0 and increases by n every 500 milliseconds:

const byN = n =>
  periodic(500).skip(1).constant(n).scan((x, n) => x + n, 0)
briancavalier commented 6 years ago

@iwinux The fix was released in most 1.5.1. Thanks again!