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

debounceTime + 0 + asapScheduler doesn't emit in special case #7205

Open
yuri-apanasik opened this issue Mar 3, 2023 · 8 comments
Open

debounceTime + 0 + asapScheduler doesn't emit in special case #7205

yuri-apanasik opened this issue Mar 3, 2023 · 8 comments

Comments

@yuri-apanasik
Copy link

Describe the bug

In a special case when we have two (or more) subscriptions on one subject with emitting to three (or more) other subjects (all subscriptions with debounceTime(0, asapScheduler)), all emissions stop after couple of steps. Prepared example looks like not real, but it is a model of a real example we have in the application (listening to form control changes with debounceTime(0, asapScheduler) to have only one emit per task, and value change can cause synchronous calculation that will change other controls).
In given example if you keep only subscription to subject1 and subject2 - works good; subject1 and subject2 and subject3 - works with a bit wrong sequence; subject1 and subject2 and subject3 and subject4 - stuck.

Expected behavior

All subscriptions "give" values, no "deadlock" in the process.

Reproduction code

No response

Reproduction URL

https://stackblitz.com/edit/typescript-evwjuc?file=index.ts

Version

7.8.0

Environment

No response

Additional context

The reason is that 'activeTask' inside 'debounceTime' stuck forever and is not realized, all new messages are ignored because new message is scheduled only "if (!activeTask)" (line 107). I am not totally sure, but I would say process looks like this:

  1. first subscription to subject1 receives the value and task is scheduled by asapScheduler with id=X
  2. second subscription also schedule task with id=X
  3. 'flush' is triggered in a state when scheduler._scheduler=X and first 'tap' executed for first subscription on subject1
  4. values to other subjects emitted in this 'tap' and tasks with other id are scheduled, scheduler._scheduled become Y
  5. action scheduled on step 2 is executed and 'emit()' in 'debounceTime' called 'activeTask.unsubscribe()'. Looks like 'unsubscribe()' triggers 'recycleAsyncId' for AsapAction and there scheduler._scheduled is set to null (line 39)
  6. when 'flash' is triggered for first task from step 4 scheduler_scheduled is null, so only one task is executed and other tasks with id=Y stays is 'actions' array
  7. finally we have 'activeTask' inside each 'debounceTime' that will never released by scheduler

I would say problem is that 'local' logic of 'debounceTime' depends on 'shared' variable '_scheduled', it is not clear why. It is ok to use '_scheduler' to decide if new task id should be planned or the same (line 22, AsapAction.ts), but this variable can be changed in different places, so when 'flash' is executed it is not guaranteed value is appropriate. I guess logic is that all actions with the same id should be executed one by one, so maybe it is better to pass 'flushId' directly to 'flush' method as an argument or take 'flushId' from first 'action' in the array.

@yuri-apanasik
Copy link
Author

I've prepared test to make a fix, finally I finished on the version with pure code without testScheduler or other helpers, here it is https://stackblitz.com/edit/typescript-gceozz?file=index.ts ('expect' part is commented and replaced with console.log).
Funny thing is that on Stackblitz it fails (as expected for now), console.log gives "'a' | 'a' | ''" instead of "'abc' | 'abc' | 'abc'", but if I put it into 'debounceTime-spec.ts' and run all tests it doesn't fail...
Any ideas why? Are there some patches for 'setTimeout/Promise/immidiateProvider/etc' that can cause difference in operators behavior in testing and runtime environments?

@yuri-apanasik
Copy link
Author

RxJS Team, I've created pull request (#7207) to master. I need your assistance to understand the problem with test (described above). If having test is not critical in current situation PR can be reviewed and merged. Also, I didn't find in contributing rules information about versions: should I cherry-pick changes and create PR to 7.x branch?

@yuri-apanasik
Copy link
Author

@benlesh, RxJS Team, any ideas regarding problem with testing environment? Could PR related to this issue be merged without test?

@kwonoj
Copy link
Member

kwonoj commented Mar 16, 2023

Could PR related to this issue be merged without test?

No.

'setTimeout/Promise/immidiateProvider/etc' that can cause difference in operators behavior in testing

This is likely nature of testscheduler only runs synchronously, so if there's inner logic involves async it may skew behaviors. Best approach is to write test without involving testsheduler if async is must to be used, or either in opposite write a test without involving async with testscheduler.

@yuri-apanasik
Copy link
Author

Hi @kwonoj, thank you for your comment. Please, check my message above (and stackblitz link)

Best approach is to write test without involving testsheduler

This is exactly what I did. Code in stackblitz gives wrong result, but if I put exactly the same code into spec file and run tests - result is different. That is the reason I can't put test into PR.

@kwonoj
Copy link
Member

kwonoj commented Mar 16, 2023

It is unclear to say why stackblitz / other runtime behaves differently. That should be clarified to check in pr: all of tests / codes runs same as long as we have same runtime (node/browsers)

@Kushalbanik262
Copy link

only having problem in stackblitz no deadlock to other js engines

@JoostK
Copy link
Contributor

JoostK commented Jul 25, 2023

I think I have observed a similar situation when using scheduled with asapScheduler, causing emissions to get stuck. The workaround was to switch to switchMap(x => from(Promise.resolve(x))) to achieve the desired coalescing effect without RxJS schedulers.

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

4 participants