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 race conditions in UseIModelData #136

Merged
merged 13 commits into from
Jan 9, 2025

Conversation

simihartstein
Copy link
Contributor

@simihartstein simihartstein commented Jan 3, 2025

The iModelGrid component was erroneously saying that I had no iModels in my iTwin. This was happening roughly 90% of the time with my initial code, but an unrelated code change could cause it to happen <5% of the time.

The race condition came in due to the useIModelData hook having multiple useEffects that relied on the same dependencies, changed the same state, where the order mattered. React does not guarantee an order in which these effects are run.

The component was refactored and simplified to have more linear logic without race conditions.

Other bugs fixed in the process of testing my changes:

  • iModels were only being fetched when the visibility of the ghost tile changed. If the query didn't result in the visibility changing (an unlikely occurrence with a page size of 100), then the iModelGrid would be stuck.
  • MaxCount was calculated incorrectly while not on the first page.

@simihartstein
Copy link
Contributor Author

simihartstein commented Jan 3, 2025

I have not tested in React 18 strict mode, but there is a chance this PR could solve #128, as that looks like it could be a race condition.

The abort controller was prematurely terminated when the dependencies of the effect were changed before the request finished.

@simihartstein
Copy link
Contributor Author

I found some bugs when the limit is set low and the imodels don't fill the page. I presume it's related to my changes even though my changes were in the data layer. Looking into it...

@simihartstein
Copy link
Contributor Author

Fixed a couple more bugs.

  • iModels were only being fetched when the visibility of the ghost tile changed. If the query didn't result in the visibility changing (an unlikely occurrence with a page size of 100), then the iModelGrid would be stuck.
  • MaxCount was calculated incorrectly while not on the first page.

Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

didnt test locally but seems ok

Copy link
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

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

didnt test locally but seems ok

@simihartstein simihartstein merged commit 0efd825 into main Jan 9, 2025
4 checks passed
@simihartstein simihartstein deleted the fix/useimodeldata-race-conditions branch January 9, 2025 03:47
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.

3 participants