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 not cleared storage task handlers from setImmediate #5016

Merged
merged 5 commits into from Dec 15, 2019

Conversation

mazulatas
Copy link
Contributor

Description:
This PR adds cleaning up handle tasks in Immediate

@mephistorine
Copy link

@cartant Why did the build fall?

@cartant cartant self-requested a review September 19, 2019 07:59
@cartant
Copy link
Collaborator

cartant commented Sep 19, 2019

Why did the build fall?

It's nothing that was done in this PR. The version of @types/node is too old for TypeScript 3.6.3. That can be dealt with elsewhere.

BTW, @mazulatas, I'll review your PR a little later. It looks good; there's just one or two things I'd like changed. Thanks.

@mephistorine
Copy link

@cartant Thanks for review, we are waiting you

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM, I'd just like to suggest a couple of name changes.

@@ -21,3 +22,9 @@ export const Immediate = {
delete tasksByHandle[handle];
},
};

export const ForTests = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd suggest renaming ForTests to TestTools and renaming numberOfTasksToHandle to pending.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree

- Reuses the same promise instead of allocating a new one on each turn.
- Uses an array to track handles rather than an object. The array should stay sufficiently small, should reduce memory pressure, and should have more consistent performance characteristics over time. The object property lookups will end up becoming slower and slower as more keys are added to it.
@benlesh
Copy link
Member

benlesh commented Sep 25, 2019

Thank you @mazulatas for catching this one and bringing it to my attention. I've further refactored the code to make it a little more efficient than it was, and I've kept your test and test util.

(I did the code editing in the browser, so we'll see if CI passes, haha)

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM

benlesh added a commit that referenced this pull request Dec 15, 2019
@benlesh benlesh merged commit 9a333f7 into ReactiveX:master Dec 15, 2019
benlesh added a commit to benlesh/rxjs that referenced this pull request Dec 15, 2019
Registered handlers would sometimes leak in memory, this resolves that issue and adds a test.

Related ReactiveX#5016
benlesh added a commit that referenced this pull request Dec 16, 2019
Registered handlers would sometimes leak in memory, this resolves that issue and adds a test.

Related #5016
@lock lock bot locked as resolved and limited conversation to collaborators Jan 14, 2020
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.

None yet

4 participants