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.

Sign up for GitHub

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

Jump to bottom

fix(scheduler): prevent unwanted clearInterval #3044

Merged
merged 2 commits into from
Nov 27, 2017
Merged

fix(scheduler): prevent unwanted clearInterval #3044

merged 2 commits into from
Nov 27, 2017

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Nov 4, 2017 •
edited
Loading

Description:

In AsyncAction, this.pending was assigned before the call to recycleAsyncId. That prevented the interval from being re-used resulting in the interval being cleared and re-created for each notification.

I cannot see how a test could be easily written for this, but the logic is straightforward.

Related issue (if exists): #3042

@rxjs-bot
Copy link

rxjs-bot commented Nov 4, 2017 •
edited
Loading

Messages
📖

CJS: 2278.8KB, global: 746.2KB (gzipped: 120.0KB), min: 145.7KB (gzipped: 31.2KB)

Generated by 🚫 dangerJS

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.218% when pulling 150a3f2 on cartant:issue-3042 into cff9dfa on ReactiveX:master.

@cartant cartant changed the title fix(scheduler): set pending after recycling fix(scheduler): prevent unwanted setInterval clearing Nov 4, 2017
@cartant cartant changed the title fix(scheduler): prevent unwanted setInterval clearing fix(scheduler): prevent unwanted clearInterval Nov 4, 2017
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.218% when pulling 5fd3ac6 on cartant:issue-3042 into cff9dfa on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Nov 8, 2017

interesting. @cartant, can you please add a test?

@cartant
Copy link
Collaborator Author

cartant commented Nov 9, 2017

@benlesh Added two tests.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.218% when pulling df89d47 on cartant:issue-3042 into cff9dfa on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.218% when pulling b9caac1 on cartant:issue-3042 into d58feb7 on ReactiveX:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 97.218% when pulling 6515461 on cartant:issue-3042 into d58feb7 on ReactiveX:master.

benlesh
benlesh requested changes Nov 9, 2017
View reviewed changes
Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit about the wording of the test.

spec/schedulers/AsapScheduler-spec.ts Outdated
@@ -40,6 +40,57 @@ describe('Scheduler.asap', () => {
sandbox.restore();
});

it('should recycle the interval for recursively scheduled actions with the same delay', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: This should probably say "reuse" rather than "recycle"... To me, "recycle" means you're tearing it down and creating another. Which is the opposite of the meaning here.

@benlesh
Copy link
Member

benlesh commented Nov 9, 2017

I'm going to have this target the 5.5 branch and cherry pick it over. We can do a patch.

@benlesh benlesh changed the base branch from master to stable November 9, 2017 18:39
@benlesh
Copy link
Member

benlesh commented Nov 9, 2017

Actually, @cartant, do you think you can rebase your changes against stable rather than master? I'd like to get this out in a patch to 5.5.

@cartant
Copy link
Collaborator Author

cartant commented Nov 9, 2017

@benlesh Rebased and tests renamed as per request.

@benlesh
Copy link
Member

benlesh commented Nov 10, 2017

Hmmm... it looks like there are some TS 2.0 incompatible lines in this already... I'm not sure I can merge this into the stable branch. :(

@cartant
Copy link
Collaborator Author

cartant commented Nov 10, 2017 •
edited
Loading

I can make the tests build on a clean node_modules install for stable if I assert to any to avoid the missing TS declaration for callThrough in the old sinon typings used by stable:

const stubSetInterval = (<any> sinon.stub(global, 'setInterval')).callThrough();

If you're okay with that, I can update the PR.

I did look briefly at installing new typings, but the typings tool used in stable - it doesn't use @types - is rather old and the two versions returned by its search appeared to be the same as those already used. I guess it could be configured to use the @types/sinon typings, but that just sounded far too tedious.

@cartant
fix(scheduler): prevent unwanted clearInterval
03d0f9f 
In AsyncAction, this.pending was assigned before the call to
recycleAsyncId. That prevented the interval from being re-used resulting
in the interval being cleared and re-created for each notification.

Closes ReactiveX#3042
@cartant
Copy link
Collaborator Author

cartant commented Nov 12, 2017

@benlesh I've made the change mentioned in the above comment and my branch's Travis build now passes - although, I had to clear the cache.

@trxcllnt
Copy link
Member

LGTM 👍

@benlesh
Copy link
Member

benlesh commented Nov 16, 2017

Yeah. I'm not sure what's going on with Travis, and I don't have time to take a look. @cartant or @kwonoj, can one of you figure this out?

@kwonoj
Copy link
Member

kwonoj commented Nov 16, 2017

@benlesh should be good now.

@coveralls
Copy link

coveralls commented Nov 16, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 97.218% when pulling 150a3f2 on cartant:issue-3042 into cff9dfa on ReactiveX:master.

@benlesh benlesh merged commit 7d722d4 into ReactiveX:stable Nov 27, 2017
kwonoj added a commit to kwonoj/rxjs that referenced this pull request Dec 3, 2017
@kwonoj
fix: Revert "fix(scheduler): prevent unwanted clearInterval (Reactive…
64f9285 
…X#3044)"

This reverts commit 7d722d4.
@kwonoj kwonoj mentioned this pull request Dec 3, 2017
Merged
kwonoj added a commit that referenced this pull request Dec 3, 2017
@kwonoj
Merge pull request #3152 from kwonoj/fix-interval
6b6843b 
fix: Revert "fix(scheduler): prevent unwanted clearInterval (#3044)"
kwonoj added a commit to kwonoj/rxjs that referenced this pull request Dec 3, 2017
@kwonoj
fix: Revert "fix(scheduler): prevent unwanted clearInterval (Reactive…
ad5c7c6 
…X#3044)"

This reverts commit 7d722d4.
This was referenced Jan 13, 2018
Closed
Merged
@dependabot-preview dependabot-preview bot mentioned this pull request Mar 14, 2018
Merged
@cartant cartant deleted the issue-3042 branch March 31, 2018 02:28
@lock
Copy link

lock bot commented Jun 5, 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 5, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benlesh benlesh benlesh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@cartant @rxjs-bot @coveralls @benlesh @trxcllnt @kwonoj