Closed matthewkeil closed 7 months ago
@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
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
deferOpen
s 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.