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

Invalid behavior of ThrottleTime operator #3712

Closed
kzaher opened this issue May 20, 2018 · 4 comments · Fixed by #5687
Closed

Invalid behavior of ThrottleTime operator #3712

kzaher opened this issue May 20, 2018 · 4 comments · Fixed by #5687
Assignees
Labels
bug Confirmed bug

Comments

@kzaher
Copy link
Member

kzaher commented May 20, 2018

RxJS version:
5.5.8
6.1.0
rxjs-compat 6.1.0

Code to reproduce:

This is RxJs behavior

5.5.8

const rx = require('rxjs');

const a = rx.Observable.timer(0, 700).throttleTime(1000, rx.Scheduler.async, { leading: true, trailing: true });
let lastDate = new Date();  
let d = a.subscribe(x => { 
    const now = new Date(); 
    console.log('\n' + x + ' ' + (now.getTime() - lastDate.getTime())); 
    lastDate = now; 
});

6.1.0

const rx = require('rxjs');
require('rxjs/operators');
require('rxjs-compat/add/operator/throttleTime');

const a = rx.timer(0, 700).throttleTime(1000, rx.Scheduler.async, { leading: true, trailing: true });
let lastDate = new Date();  
let d = a.subscribe(x => { 
    const now = new Date(); 
    console.log('\n' + x + ' ' + (now.getTime() - lastDate.getTime())); 
    lastDate = now; 
});

Expected behavior:

This is underscore:

const underscore = require('underscore');
const rx = require('rxjs');

let lastDate = new Date();  
d= rx.Observable.timer(0, 700).subscribe(underscore.throttle(x => { 
    const now = new Date(); 
    console.log('\n' + x + ' ' + (now.getTime() - lastDate.getTime())); 
    lastDate = now; 
}, 
1000, 
{ leading: true, trailing: true }))
11 1003
12 1002
/// missing 13, all intervals above 1 sec
14 1001

This is RxSwift behavior

var last = Date();
        _ = Observable<Int>.timer(0, period: 0.7, scheduler: MainScheduler.instance)
            .throttle(1.0, latest: true, scheduler: MainScheduler.instance)
            .subscribe({ next in
                let now = Date();
                print("\(next) \(now.timeIntervalSince(last))");
                last = now;
            })
next(11) 1.00001406669617
next(12) 1.00061988830566
// missing 13, all intervals above 1 sec
next(14) 1.00011909008026

Actual behavior:

11 1000
12 407 // <--- time interval less then 1 sec
13 1004 // <--- has 13
14 402

Additional information:

The operator throttleTime is not behaving as I would expect. If I set throttle interval to 1sec I should never receive elements under 1 sec.

@benlesh benlesh added the bug Confirmed bug label May 21, 2018
@bbonnet
Copy link
Contributor

bbonnet commented May 23, 2018

I was able to reproduce this behavior and implement a failing marble test case that I think matches what you're seeing. I can use some help confirming that this is indeed the expected behavior:

const dueTime =  time('|');
const period  =  time('---|');
const throttle = time('-----|                  ');
const expected =      'a----b----c----d----e(f|)';

const source = timer(dueTime, period, rxTestScheduler).take(8).throttleTime(throttle, rxTestScheduler, { leading: true, trailing: true });
const values = { a: 0, b: 1, c: 3, d: 4, e: 6, f: 7};
expectObservable(source).toBe(expected, values);

Want to make sure this is correct before attempting to implement a fix.

@kzaher
Copy link
Member Author

kzaher commented May 24, 2018

Hi @bbonnet ,

what does (f|) mean? I'm not sure that e and f should be that close.

@bbonnet
Copy link
Contributor

bbonnet commented May 25, 2018

f, the last value is emitted immediately upon completion.

So this means that the throttle timer would emit e and then complete happens right away and then f is emitted. That's why the end up being so close.

@sod
Copy link

sod commented Oct 19, 2018

Tried to fix that in #2749 but gave up getting it into rxjs and just use my own operator.

@benlesh benlesh self-assigned this Sep 2, 2020
benlesh added a commit to benlesh/rxjs that referenced this issue Sep 2, 2020
…least the throttled amount

Works to align the behavior with expectations set by lodash's throttle

- Updates tests
- Ensures trailing throttle will wait to notify and then complete
- Ensures that every time we emit a value a new throttle period starts

fixes ReactiveX#3712
related ReactiveX#4864
fixes ReactiveX#2727
closes ReactiveX#4727
related ReactiveX#4429
benlesh added a commit to benlesh/rxjs that referenced this issue Sep 3, 2020
…least the throttled amount

Works to align the behavior with expectations set by lodash's throttle

- Updates tests
- Ensures trailing throttle will wait to notify and then complete
- Ensures that every time we emit a value a new throttle period starts

fixes ReactiveX#3712
related ReactiveX#4864
fixes ReactiveX#2727
closes ReactiveX#4727
related ReactiveX#4429
benlesh added a commit that referenced this issue Sep 3, 2020
…least the throttled amount (#5687)

* fix(throttleTime): ensure the spacing between throttles is always at least the throttled amount

Works to align the behavior with expectations set by lodash's throttle

- Updates tests
- Ensures trailing throttle will wait to notify and then complete
- Ensures that every time we emit a value a new throttle period starts

fixes #3712
related #4864
fixes #2727
closes #4727
related #4429

* chore: Address comments and add comments to the code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment