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

Break potential infinite loop #71

Merged
merged 4 commits into from
Jun 28, 2021

Conversation

eliranek1
Copy link
Contributor

Summary
There is a race condition case where we're reaching an infinite loop when rebalancing. This only happens sometimes so it's hard to repro.

Tracked down the issue to isActive always being true in the rebalance while loop so it never exits.

IsActive: Path !== PathNode.NONE
Release: Set Path.Node = NONE & Path.Parent = Cache

However, what's happening in the repro is that there is a "next" branch set on the path which causes isActive to always return true and never get out of the loop.

Set a breakpoint in the isActive check in the while loop:
image

Notice that next is set on both parent path and the this path despite it getting released.

@axefrog
Copy link
Member

axefrog commented Jun 26, 2021

Hey there, thanks for the PR! As you can see, I haven't worked on this library in quite a few years. In fact I just noticed I accidentally missed the other PR as well (submitted in 2019). I am happy to merge yours, but I am wondering if you could take a quick look at the other PR and tell me if I should merge that one before I merge yours? I don't even have this repository checked out right now, and the code is definitely not fresh in my mind ;-)

@eliranek1
Copy link
Contributor Author

eliranek1 commented Jun 28, 2021

@axefrog, I took a look at the PR and it seems ok, I don't see a specific need for it though or what use case it's solving. Also, I would prefer to have UTs for this before it's checked in. I left a comment. I would prefer to hold off on that if it isn't required.

For now, do you mind doing a closer review of mine and merging in if you don't see any issues with setting next to branch none when we reset?

@axefrog axefrog merged commit afc853c into frptools:master Jun 28, 2021
@axefrog
Copy link
Member

axefrog commented Jun 28, 2021

I have merged. I'll try to find some time to deploy a new copy to npm. May I ask, are you using this library a lot? Or just for a task you are working on?

@eliranek1
Copy link
Contributor Author

Thank you! Yes, we are using it quite extensively in our datastore. Specifically, the RB Tree as the structure for our in-memory provider:
https://github.com/microsoft/ObjectStoreProvider

@axefrog
Copy link
Member

axefrog commented Jun 29, 2021

Ah cool! The reason I asked was that I'd be open to a co-maintainer on the project, seeing as I'm very inactive with it at present. I may have time for it in the future, but right now I am focused on another big project, and this one has fallen by the wayside, as you can tell by the project's inactivity over the past few years. No pressure, just throwing it out there...

@eliranek1
Copy link
Contributor Author

@axefrog, I would love to! 👍

By the way, I missed upgrading the semver in package.json so the update never got published. I'll send one out in a few mins and add my name to the authors list.

Thanks! Excited to work on this!

@axefrog
Copy link
Member

axefrog commented Jul 4, 2021

Hey, sorry for the long delay in reply. There are other, quicker ways to contact me if you'd like. My username also works with Gmail, Google Chat/Hangouts and Skype. I'm also on Discord if you prefer. Anyway, I have just published to npm. Contact me on another medium and I can give you npm publishing credentials, and we can have a quick chat about the setup I was using, if it would help. Also should I add you as a collaborator to the collectable repository, or do you prefer to go about this another way?

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

Successfully merging this pull request may close these issues.

2 participants