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

fix(debounceTime): synchronous reentrancy of debounceTime no longer swallows the second value #3218

Merged
merged 1 commit into from
Jan 8, 2018

Conversation

jayphelps
Copy link
Member

@jayphelps jayphelps commented Jan 8, 2018

Currently the second value is swallowed because hasValue is set to false in between the scheduling of the second and its execution.

fixes #2748

const results = [];
const source = new Rx.Subject();
const scheduler = new VirtualTimeScheduler();

source.debounceTime(0, scheduler).subscribe(value => {
  results.push(value);

  if (value === 1) {
    source.next(2);
  }
});
source.next(1);
scheduler.flush();

expect(results).to.deep.equal([1, 2]);

@kwonoj you had mentioned in #2748 that rxjs doesn't guarantee reentrancy, though I think in this case the fix is simple. Are there reasons to explicitly not do it in this case?

FWIW the regular debounce operator already supports reentrancy, so I added a test for it too. This all seems so familiar so perhaps we discussed this years ago and fixed only debounce lol

@kwonoj
Copy link
Member

kwonoj commented Jan 8, 2018

@jayphelps as long as it doesn't hurts current code execution, I don't think there's strong reason to explicitly block changes to allow reentrancy. Still in v5 without scheduler reentrancy is not gauranteed, means might work but mostly not work in general - not sure just fixing single operator gives huge value. If someone chains another operator doesn't work with reentrancy with fixed debouncetime, then consumer still will be blocked and have to reschedule it.

@rxjs-bot
Copy link

rxjs-bot commented Jan 8, 2018

Messages
📖

CJS: 1384.2KB, global: 753.1KB (gzipped: 120.8KB), min: 145.3KB (gzipped: 31.4KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 97.109% when pulling 1050a5d on jayphelps:debounceTime into c1721e3 on ReactiveX:master.

@jayphelps
Copy link
Member Author

Hmm Travis can’t seem to find Node v4. Maybe they removed it? I googled a bit, can’t find confirmation

@kwonoj
Copy link
Member

kwonoj commented Jan 8, 2018

🤔 weird, nvm seems failing to look it up but I also couldn't see anything related.

@kwonoj
Copy link
Member

kwonoj commented Jan 8, 2018

seems it was one time glitch. restarting job makes it work.

@benlesh
Copy link
Member

benlesh commented Jan 8, 2018

LGTM

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

debounceTime dropping values received in subscription
5 participants