Assemblymanager thread safety #277
Conversation
|
This is failing on Travis CI (Linux) for 2 Python versions on your added assert test. Concurrent list is not possible by design, but can't we use anything else from System.Collections.Concurrent or at least subclass list with locked methods? http://stackoverflow.com/a/9995339/2230844 This is to prevent manual lock maintenance, especially when people would not be aware of multi-threading support. |
|
Thanks for the suggestions: I will fix the test to be more predictable and not have any collateral impact on other tests and create a little wrapper for List<>. |
…n test_AssemblyLoadThreadSafety
|
@tonyroberts @filmor @vmuriart this pull request looks good to me. please review! @ArvidJB is the list with locks your own implementation or you copied it from somewhere else? |
|
@denfromufa that List is my own implementation (although I did read through the stackoverflow answers and some related discussions to come up with it). |
|
then is it possible to avoid using SupportsRecursion? It is not recommended https://msdn.microsoft.com/en-us/library/system.threading.readerwriterlockslim.aspx On Thu, Oct 27, 2016 at 12:46 PM, ArvidJB notifications@github.com wrote:
|
|
@denfromufa good point, I removed SupportsRecursion. I also added a read lock around _list.Count which I had missed earlier. |
|
@ArvidJB can you add more imports in the test? "import clr" is very special compared to imports of Python modules and .NET assemblies. |
|
@denfromufa sure, added some more imports. I only added "import clr" because that's what caused our problems with its call to GetNames(), but the other code paths potentially could also have race conditions. |
| @@ -66,7 +66,8 @@ def testModuleInterface(self): | |||
| self.assertEquals(type(System.__dict__), type({})) | |||
| self.assertEquals(System.__name__, 'System') | |||
| # the filename can be any module from the System namespace (eg System.Data.dll or System.dll) | |||
| self.assertTrue(fnmatch(System.__file__, "*System*.dll")) | |||
| self.assertTrue(fnmatch(System.__file__, "*System*.dll"), | |||
| "unexpected System.__file__" + System.__file__) | |||
There was a problem hiding this comment.
@ArvidJB can you add a comment why this was added in test?
There was a problem hiding this comment.
The System namespace is spread out across a couple of DLLs: mscorlib.dll, System.dll, and all the other System.*.dlls. In ModuleObject we iterate through all the known assemblies for a namespace and pick the last one for the filename. The iteration order would be the on the hash of the assembly, so it should have always been unpredictable whether you get mscorlib.dll or one of the others.
Let me change the test such that mscorlib.dll is also accepted.
There was a problem hiding this comment.
PS: I added the error message because I could not reproduce this on my machine and wanted to know what value it got in the integration tests.
|
@tonyroberts @filmor @vmuriart this pull request looks good to me. please review, at least one of you! @ArvidJB thanks for putting this together! |
|
@denfromufa thanks for reviewing! |
|
The last commit confused me, did we merge |
|
I updated this branch with the master to re-run the tests. On Tue, Nov 1, 2016, 6:09 PM Vik notifications@github.com wrote:
|
|
@ArvidJB I will wait couple days during release of pythonnet v2.2.0 before merging this. This also gives some time to @filmor @tonyroberts in case they would like to review this. I also need to check embedding tests, which are currently not setup in CI. |
…vidJB/pythonnet into assemblymanager_thread_safety
|
I did review this, didn't see any glaring errors. But this threading stuff is hard :) How about using SynchronizedCollection instead of rolling your own implementation? |
|
@filmor I would have preferred not to write this myself but I did not find a good match in the .NET framework. SynchronizedCollection is a weird class, it's in System.ServiceModel for some reason. And it does not provide thread-safe enumeration: |
We observed a spurious crash when a spawned .NET thread loaded some additional assemblies triggered the AppDomain.AssemblyLoad event handler. This modified the namespaces Dictionary and led to an exception
when we called import clr again from the Python thread. I was able to write a test which reproduces this (most of the time).
As a fix I propose to make namespaces a ConcurrentDictionary. The same issue should also apply to the assemblies list. Unfortunately there is no ConcurrentList, so I added a lock. All the operations under the assemblies lock seem simple enough to not cause any deadlocks by triggering the same codepath on a different thread.