test: fix race condition #90
Conversation
|
@vweevers I found the race. It was a "hidden" async open in abstract-level that gets called by default from the synchronous constructor. |
|
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 |
|
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!! |
|
<< blows into hand and inhales >> Is it my breath? |
|
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! |
|
I am waiting for this too |
There was a problem hiding this comment.
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!
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
openensures 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.