Debounce repeats last emission at completion

[rhartvig]

Debounce operator emits last value at completion, even though no event is delayed.

This is apparently due to the timer not being reset after firing: https://github.com/cujojs/most/blob/a88c3169d231bcfe6f780cbd1db94b3b304ff284/src/combinator/limit.js#L92

On call to end, this is interpreted as a pending event.

Expected result

Once a debounced event has fired, it should not fire on stream completion.

Actual Result

See codepen: https://codepen.io/corolla/pen/XVwGQY?editors=1012#0

Notice 'foo' outputted twice indicating emission on stream completion.

Versions

• most.js: 1.7.2
• Chrome 63

Steps to reproduce

Run code, observe foo emitting twice.

Code to reproduce

most
  .never()
  .startWith('foo')
  .until(most.of().delay(1000))
  .debounce(100)
  .subscribe({
    next: console.log,
    complete: () => console.log('complete')
});

[briancavalier]

Hey @norpoint, thanks for reporting this, and for the test case! I'll start digging into it.

[briancavalier]

apparently due to the timer not being reset after firing

Yep, you're right. In this particular case, the fact that the end is delayed beyond the debounce window exposes the problem (since there's no end to clear the timer before it fires). I pushed a candidate fix in #515. Could you try the branch and let me know if it works for you? Thanks!

[rhartvig]

Perfect, thanks @briancavalier , and sorry for the late reply. Works, gj.

[rhartvig]

@briancavalier , sorry for chiming in late, but a suggestion: It seems now that the debounced value is stored in 2 places, making it slightly harder to reason about the code. Would it make sense to either store value only in DebounceSink or DebounceTask? Going with the former and:

DebounceSink.prototype._event = function (t) {
  this._clearTimer()
  this.sink.event(t, this.value)
}

Or with the latter and:

DebounceSink.prototype.end = function (t, x) {
  if (this.timer) {
    this.timer.run()
  }
  this._clearTimer()
  this.sink.end(t, x)
}

Both seems to work (though the latter doesn't clear the value after end).

[briancavalier]

@rhartvig Cool, glad that works for you :+1:

Yeah, I debated something similar, too. I'll try to explain why I went with the current approach. The value stored in DebounceSink is mutable, and tracks the latest seen value, and the one stored in DebounceTask is immutable and represents the pending debounce value. The use case of the former is only for end, and the use case for the latter is to implement the actual debounce functionality. Those are subtly different intents, and I thought keeping them separate might be better.

That said, I think your idea is interesting. I hadn't considered invoking the task manually. I'll give it a bit more thought today.

In the meantime, I think we'll release this as is, to get the bug fix out. If we decided to change the implementation, we can do that in another release.

[rhartvig]

@briancavalier , thanks, really interesting to hear these considerations, and it totally makes sense to go ahead with the bug fix as is.

The immutability gained from storing in DebounceTask certainly certainly has an appeal. Pursuing that option, you could consider shaving off the last _clearTimer, e.g.:

if (this.timer) {
  this.timer.run()
}
this.sink.end(t, x)

Untested, but I guess this would still clear the timer. I initially considered it a bit contrived though. Anyway, for later.

[briancavalier]

Both seems to work (though the latter doesn't clear the value after end)

Interestingly, I think it does, transitively. Since the DebounceTask would hold the only reference to the value, _clearTimer nulls this._timer, that's sufficient to release Debounce's reference to both the DebounceTask and the value.

Untested, but I guess this would still clear the timer.

Hmmm, this is a good point. It actually won't :). Since the DebounceTask has been scheduled (there's a reference to it in the scheduler's internal timeline data structure), we'd need to unschedule it manually by calling this.timer.dispose(). Otherwise, the scheduler has no way of knowing someone manually called run on it with the intent of also unscheduling it. We'd probably also want to this.timer = null to be as gc friendly as possible.

[rhartvig]

Interestingly, I think it does, transitively

Good point. No need for anything else than gc.

we'd need to unschedule it manually

Wouldn't that happen as part of run() -> _event() -> _clearTimer() -> timer.dispose() ?

[briancavalier]

Wouldn't that happen as part of run() -> _event() -> _clearTimer() => timer.dispose() ?

Aha! Yes, you're right. Thanks.