-
Notifications
You must be signed in to change notification settings - Fork 4
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
Remove //base
threading & scheduling dependency
#13
Labels
threading
//base threading infra
Comments
This was referenced Aug 24, 2023
domfarolino
added a commit
that referenced
this issue
Sep 10, 2023
domfarolino
added a commit
that referenced
this issue
Sep 19, 2023
Makes progress on #13. This PR removes the final usage of `CHECK_ON_THREAD()` from `Node`, which was in `PrepareToForwardUserMessage()`. This PR: 1. Removes that usage, and the `thread_checker.h` include 2. Refactor's the signature of `PrepareToForwardUserMessage()` to make it a little more self-documenting 3. Adds more documentation to that whole method, to better describe its flow and how it is used.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
As described in the Mage documentation, specifically in https://github.com/domfarolino/mage#threading--task-scheduling, Mage has a strong dependency on the
//base
threading & scheduling library which was written with Mage in mind. We have a goal of decoupling this library from//base
for this reason://base
library for all of their scheduling — the integration cost would be too high, and that's generally a bad design goal.Technically we don't have to remove full reliance on the
//base
library; if Mage continues to depend on//base
for random utilities likeCHECK()
, that wouldn't prevent the project from being useful and moving forward — it would just be unnecessary. So specifically, the goal is to remove Mage's reliance on the threading & scheduling components of//base
(which of course is the primary thing that library is responsible for, but still being precise about this is important).Decoupling Mage from
//base
requires making Mage generically thread-safe. Today, Mage is only partially thread-safe. Specifically:Core::handle_table_
, which can be accessed from any thread//base
; we assert that some methods are only ever called on certain well-named threads, so we don't require locks in some places, because the assertions ensure some data structures are only ever handled on threads that are implicitly safeCreateMessagePipesAndGetEndpoints()
is an example of this, where we modifyNode::local_endpoints_
without locking access to it, because we only ever add to it on the main thread. Even this might not be fully true actually, but that is the intent, which is simply insufficient once these sorts of operations can truly be called from arbitrary threads (within what the documented Mage API allows for)//base
dependency; these are just bugs plain and simpleNode::pending_invitations_
map: insertions are made on the UI thread when an invitation is sent, and removals/look-ups happen on the IO thread when acceptance is received. For a single invitation this is "safe" because receipt of an invitation can only ever follow the sending of an invitation (which includes the insertion which must be finished at that point). But when sending many invitations, some acceptances can be received while others are still being sent. This leads to thread-unsafe mutations of the mapChannel
's use ofwrite()
is not locked based on socket. This means multiple messages being sent in parallel to the same socket (bound for different endpoints, perhaps) can lead to corrupted/interleaving messagesIn order to decouple Mage from the threading & scheduling infrastructure that
//base
provides, we have to fix the 🟠 and ❌ cases — all of them, not just the examples listed above — and at the same time document which methods of the Mage public API are thread-safe, vs. which group of APIs must be called on the same thread (if any of these exist).The text was updated successfully, but these errors were encountered: