Extract a TaskQueue class #5477
Conversation
There was a problem hiding this comment.
I think this is fine. What follows is me thinking out loud about this:
I understand the thread-safety macros like GUARDED_BY, but those are only helpful if we were running things like ThreadSanitizer or Helgrind (which AFAIK we are not).
One technique that I've used in the past is that everything that gets called with a lock held must also take (in absl's case) a const MutexLock& indicating that you called it with the appropriate lock held. IMHO, this makes it much clearer what's going on: the lock-edness is seen at the point of use of the relevant variables, rather than having to investigate whether the variable has a GUARDED_BY at the declaration site.
It's not a perfect system, of course: you can write new member functions that don't take const MutexLock & parameters, but do access locked data. Or you write new public interfaces that wouldn't take const MutexLock & (because you would expect callers to not be holding the lock) and then forget to do the locking. Or you can make other kinds of mistakes. But my experience is that this style is really helpful for figuring out what's going on.
This technique falls down a little bit if you have multiple locks that need to be held simultaneously from the same class (how do you know which MutexLock corresponds to which lock?), but I could imagine a newtype-style class for locking a specific mutex, so you would have a different type for each mutex.
Anyway, not saying that we have to convert TaskQueue to this system, but it is something to think about; pushback on the idea welcome.
We discussed this a bit in a DM, and since clang will check these annotations at compile time I think they're worth keeping. Separately, newtyping the locks and passing them in as arguments also seems like a great api design to me and I'd be happy to switch to that. You're right that we'd likely have issues once we're looking at coordinating multiple locks, but we could always extend the newtyping to include multiple locks to name the strategy. For now I think I'll land this change as-is, and we can discuss the better long-term api design changes :) |
👍 I had not known the annotations were checked at compile time, that makes things much more interesting. For those interested, the compiler bits behind this are at: https://clang.llvm.org/docs/ThreadSafetyAnalysis.html |
|
We have a policy of testing changes to Sorbet against Stripe's codebase before Stripe employees can see the build results here: → https://go/builds/bui_LL70Bd3OdZKtyE |
Extract a
TaskQueueclass from what was previously a pair of a mutex and theTaskQueueStatestruct. This is prework for moving the indexer thread's initialize behavior into the typechecker thread, as the typechecker will need a handle to the task queue to feed the results back through to the preprocessor thread.Motivation
Prework for reworking how
GlobalStateinitialization happens.Test plan
See included automated tests.