-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
concurrent loading does not resolve loaded constant #4091
Comments
It looks to me like there's a bug in how we lock down concurrent requires. I suspect the failing thread is passing over the require call before it is complete, proceeding to access constants that are not there. I'll be poking at this today. |
that's what I thought but the thread doing the require than fails resolving the constant. |
Are you sure? Here's output from my run, logging each thread before it attempts to read or write
This shows three non-main threads attempting to load that file and access that constant. Notice that "71" and "17" say they're going to access The only way we'd see "71" and "17" proceed to access without trying to define is if require either told them the file was already required (concurrency problem with loaded features?) or if it allowed them to proceed while the require was still in progress. I suspect the latter, since loaded features shouldn't be updated until the load/require has completed. |
On a hunch, I ran with -w. The results are very interesting:
It seems like the locking logic for require is treating concurrent requires as circular rather than conflicting, and sending the calling thread on its way with nothing but a stern warning. |
And a bit more logging around the require lock confirms it:
Here we see thread 1 attempt to acquire the lock, and then 2 and 3 follow it. Both 2 and 3 then follow the path that leads to circular warning (requireLocks.lock returned false rather than blocking). |
I should have examined this commit more closely: 455bcf0 This is the culprit, and potentially the cause of many other require-time threading bugs we've seen. This lock can't be a tryLock because we use a false return value here to indicate circular requires. It may have prevented some deadlocks, but only by allowing everyone to ignore the require lock altogether. We obviously need better tests for this. |
Ref #3341 from the bad commit. |
This also appears to be the cause of other circular require warnings, like those seen in |
eh, sorry for the confusion I did not use |
seems to be a regression compared to 1.7 or even 9.0.5
a bit tricky - so far I have been only able to do a reproduction case that 'sometimes' fails :
https://gist.github.com/kares/596f5a3648249181373dca2242332613
HINT: loading seems to always fail from the same thread as the one first hitting the
require
.The text was updated successfully, but these errors were encountered: