Make mono.addins thread safe #187
Conversation
| list.CopyTo (array, index); | ||
| } | ||
|
|
||
| IEnumerator<ExtensionNode> IEnumerable<ExtensionNode>.GetEnumerator () |
There was a problem hiding this comment.
Would probably help a tiny bit to have a struct enumerator pattern.
slluis
changed the title
Remove the concept of TreeNodeBuilder, it was getting too complex. An easier solution is to add a transaction mode to TreeNode, which when enabled handles children in a list builder, and which is committed when ending the context transaction. Fixed several bugs.
And fixed threading issue.
Store extension content data in a snapshot class, so that it is possible to swap it all at once when committing a transaction. Propagate transactions to more methods. Events and virtual methods are now guaranteed to be executed sequentially, and never concurrently. Add threading docs.
slluis
force-pushed
the
dev/lluis/threadsafe
branch
from
cd227db
to
377ecba
Compare
|
I still haven't had a chance to review this, sorry! |
Reduce the number of transactions being created by propagating them. Created a transaction when the engine is initialized, so that the initial loading of add-in roots is all done using a single transaction. Fixed unit tests.
| @@ -345,7 +355,7 @@ Type GetType_Expensive (string typeName, string assemblyName) | |||
|
|
|||
| foreach (var kvp in loadedAssemblies) { | |||
| var assembly = kvp.Value; | |||
| if (string.IsNullOrEmpty (assemblyName) || assembly.FullName == assemblyName) { | |||
| if (string.IsNullOrEmpty (assemblyName) || assembly.GetName().Name == assemblyName) { | |||
There was a problem hiding this comment.
Why are we choosing the short name here?
There was a problem hiding this comment.
Honestly, I don't remember. I'll take a look.
There was a problem hiding this comment.
It seems like I did this change because Util.TryParseTypeName() does return the short assembly name: https://github.com/mono/mono-addins/blob/main/Mono.Addins/Mono.Addins.Database/Util.cs#L82, so without this change assembly names would never match. I guess I fixed this long time ago since I don't remember the issue. I wonder how this is affecting VS Mac.
There was a problem hiding this comment.
AssemblyQualifiedName should give us a full name for the assembly name.
typeof(int).AssemblyQualifiedName
System.Int32, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e
I think the problem is that we're wrapping it in a new AssemblyName and returning the name, because we should be able to use the fullname here.
There was a problem hiding this comment.
Basically, the part that we're passing to new AssemblyName is System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e. I'm wondering why we drop that, because we should preserve that information. If we want to have multiple AssemblyLoadContexts available, we need that information.
|
Mostly LGTM! |
Split the context transaction class in two classes, one for extension context and one for add-in engine (which is a context by itself). In this was there is no danger of providing a context transaction to an add-in engine method and expect it to work. Added method for stating an engine transaction from a context transaction. Fixes tests.
|
|
||
| internal class AddinStatus | ||
| { | ||
| public AddinStatus (string addinId) | ||
| public AddinStatus (string addinId, bool configEnabled = false, bool? sessionEnabled = null, bool uninstalled = false, ImmutableArray<string> files = default) |
There was a problem hiding this comment.
One thing we could try is using record here, because that gives you implicit immutability with init-only properties.
It also provides a base hashcode/iequatable, and mutating via: original with { SomeProp = newValue }.
There was a problem hiding this comment.
Less boilerplate overall :D
Makes Mono.Addins safe to use from multiple threads. Most internal collections have been converted to immutable, so normal access to the extension tree is lock-free, so it should be fast. Locks are acquired when the tree is built (on demand).
Some noticeable changes:
Mono.Addins is now thread safe, meaning that it is possible and safe to use
AddinManager,AddinEngineand related classes to access the extension model from different threads.Mono.Addins by itself does not use multiple threads. If the add-in engine is always used from a single thread, then the behavior is the same as it used to be before thread-safety support. There are however several considerations to take in to account when using Mono.Addins in a multi-threaded application.
Extension model queries
The extension model is a mutable data structure. Extension nodes can be added or removed as a result of conditions changing or add-ins being enabled or disabled. Methods like
ExtensionContext.GetExtensionNodes()return a snapshot of the nodes at the time of the call, and that snapshot won't change. However, consecutive calls to that method may return different results if the extension model mutates between calls.Event Handlers
The
AddinManager,AddinEngine,ExtensionContextandExtensionNodeclasses have events that are raised when the extension model changes. In a multi-threaded application, those events may be raised in different threads, depending on which thread the change was originated. Subscribers of those events must take that into account and take thread safety measures by themselves if necessary.In any case, Mono.Addins guarantees that events for a specific extension context are raised sequentially. For example, if a child node is added and removed from a node, the corresponding Add event handler will be executed first, and when it completes the Remove handler will be executed next.
Custom extension nodes
It is up to Mono.Addins extenders to guaratee the thread safety of custom extension nodes. The implementation must take into account that nodes can be created in different threads, and that virtual methods such as
OnChildNodeAddedorOnChildNodeRemovedcan also be invoked from different threads (those methods however are executed sequentially, like explained above for events).Conditions
Custom conditions may need to be evaluated from different threads, so the
ConditionType.Evaluate()method should be thread safe. TheConditionType.NotifyChanged()method, used to notify that the condition has changed, can be invoked from any thread. Extension model change events caused by condition changes will be raised in the thread that invokedNotifyChanged().