-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Race condition when registering ATC tables #8323
Comments
I did some digging as well and think this is on the right track, with a caveat that the error from trying to attach the table in the ATC plugin too early is dropped here (ie in the ATC plugin code). The multiplexing code in the registry factory that drops errors only happens if there are multiple active plugins separated by a comma, but the call to the SQL plugin specifically just requests In any case, the Table plugin registry needs to be aware of a table before the "attach" request is made to the SQL plugin, meaning #8233 should be reverted in favor of an alternate fix for the duplicate ATC table registration issue. Besides the ATC plugin, there are two other paths that add tables to the registry and SQL connection:
Because ATC tables can be added and removed dynamically (including in the race with table extensions described here), they need to call SQL attach explicitly -- only the built-in tables can safely rely on database initialization to attach the tables. There are a couple paths on how to fix both this bug and the duplicate ATC table registrations:
I think the second is a much simpler fix without much downside. I'll put up a quick PR as an example, not sure how much time I'll have to actually test it |
|
OK I decided to go with the pending state idea instead, as it seems more correct/less hacky. I think forcing initialization in the ATC table would also mostly work, but in theory there could be (admittedly very unlikely) races where the database gets reinitialized in the middle, causing a duplicate registration again. Here's a PR: #8324 |
It seems like #8233 has introduced a race condition when attempting to register both ATC tables and an extension at the same time.
The issue the PR was fixing was the first ATC table would attempt to generate twice, and therefore was causing an error message. The fix here was moving the
Registry::call
for attaching the ATC tables plugin to before the point where the table plugins are added to the registry.The reason this seemed to work is because we drop the error:
return Status::failure("Cannot call registry item: " + item_name);
returns to:Yes the
Registry::call
fails but theRegistry::add
doesn't, soattachTableInternal
picks it up and generates the tables.However, when an extension comes into play the extension registration may or may not run before the ATC registration. If it does run first, then it pushes out the ATC registration, which leaves nothing to attach the ATC tables since the
Registry::call
failed and they register afterattachTableInternal
has ran.ATC registers first and is a success:
Extension registers first which causes the ATC tables to not get picked up by
attachTableInternal
:The text was updated successfully, but these errors were encountered: