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

test: fix race condition #90

Merged
merged 2 commits into from
Jan 20, 2024
Merged

Conversation

matthewkeil
Copy link
Contributor

@matthewkeil matthewkeil commented Dec 1, 2023

In one unit test there were two sync constructors next to each other that were each calling async open through a nextTick in abstract-level here. The next line of the unit test called that same function to check that it errored.

The initialization is managed through events and the race was in the open resolution and event reporting between the several calls to "open" which sometimes was caught as as explicit call in a try/catch and sometimes not as a nextTick call from the constructor.

Moving the constructor into the try/catch and awaiting the second call of open ensures that the race is caught correctly in the unit test.

The only way to "fix"/remove the race would be to breaking-change the way constructor deferOpens the db by defaulting to false. That will break the behavior for all consumers that rely on that behavior.

It seems unlikely for anyone to put two constructors next to each other so I recommend not changing the behavior and I simply updated the unit test to check for how the library would most likely be used in practice.

@matthewkeil matthewkeil marked this pull request as ready for review December 1, 2023 03:42
@matthewkeil
Copy link
Contributor Author

@vweevers I found the race. It was a "hidden" async open in abstract-level that gets called by default from the synchronous constructor.

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Dec 6, 2023

A friendly ping @vweevers. Also curious if you have any thoughts @ralphtheninja. Would love to get this published so we can import it into ChainSafe/lodestar#6033

@matthewkeil
Copy link
Contributor Author

Merry Christmas @vweevers!! I hope this message finds you well. Please feel free to ask me for any clarifications or for my findings from researching this issue and I will be more than happy to oblige. My project manager @philknows was asking about where we stand with this and I would love to report back to him when we reconvene after the new years. Thanks again and have a very happy holidays!!

@matthewkeil
Copy link
Contributor Author

matthewkeil commented Jan 2, 2024

<< blows into hand and inhales >> Is it my breath?

@philknows
Copy link

philknows commented Jan 16, 2024

Hey @vweevers and @ralphtheninja! Hope all is well. I was just wondering if you happened to see this? It's currently blocking one of our features and would be awesome just to get a quick response on this PR if possible. We'd be happy to fix this if there's any issue with the PR. Otherwise, we may end up having to fork this with the fix, thanks for your time!

@xrchz
Copy link

xrchz commented Jan 19, 2024

I am waiting for this too

Copy link
Member

@vweevers vweevers left a comment

Choose a reason for hiding this comment

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

I agree with the fix. It was a bug in the tests. The behavior of the ClassicLevel constructor is correct because if two ClassicLevel instances try to asynchronously open the same LevelDB database then only one can win. The only thing we might want to improve later is the error message (Database is not open is too generic) once someone has a real use case for two instances in the same worker/thread. But I doubt it, so we're done. Thanks!

@vweevers vweevers merged commit 9ff2e82 into Level:main Jan 20, 2024
15 checks passed
@vweevers
Copy link
Member

1.4.1

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.

4 participants