Skip to content

Fixes lock recursion bug in assembly loading #628

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

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Fixes lock recursion bug in assembly loading #628

merged 1 commit into from
Feb 26, 2018

Conversation

Cronan
Copy link
Contributor

@Cronan Cronan commented Feb 14, 2018

What does this implement/fix?

The AssemblyList class has a bug where assembly loading race conditions can cause a LockRecursionException under certain circumstances. Replacing the custom implementation with a ConcurrentBag<Assembly> solves the problem.

Does this close any currently open issues?

This closes #627

Any other comments?

The downside of ConcurrentBag is that it doesn't support accessing items via the index, and it is an unordered collection, neither of which matters in the way we are using it.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@codecov
Copy link

codecov bot commented Feb 14, 2018

Codecov Report

Merging #628 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #628      +/-   ##
=========================================
- Coverage   76.99%   76.9%   -0.09%     
=========================================
  Files          64      64              
  Lines        5612    5582      -30     
  Branches      888     888              
=========================================
- Hits         4321    4293      -28     
+ Misses       1002    1000       -2     
  Partials      289     289
Flag Coverage Δ
#setup_linux 69.42% <ø> (ø) ⬆️
#setup_windows 76.08% <100%> (-0.1%) ⬇️
Impacted Files Coverage Δ
src/runtime/assemblymanager.cs 89.1% <100%> (-0.69%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c18285f...adc1240. Read the comment docs.

@den-run-ai
Copy link
Contributor

@Cronan related PR from @ArvidJB, where he introduced AssemblyList:

#277

@den-run-ai
Copy link
Contributor

@Cronan I'm all for not using our own implementation of concurrent collection and use .NET types :)

Here is one case to consider:

             var names = new List<AssemblyName>(assemblies.Count);

Is it possible that new elements are added to assemblies concurrently before the next line with iteration starts and hence .Count is not correct for the size of names anymore?
This may sound crazy scenario, but I would hate to debug concurrency bugs.
So maybe add a lock around this?

@ArvidJB
Copy link
Contributor

ArvidJB commented Feb 16, 2018

@denfromufa that line is just a hint to pre-allocate assemblies.Count slots in the List and if you get that wrong the only implication is that it needs to reallocate the internal array in the List.

I think the only real difference here is that ConcurrentBag does not try to maintain any ordering of the elements you add. The ordering of the assemblies would dictate the resolution for classes which are present in multiple assemblies (not sure how frequent this actually is). Switching the order during the evaluation of the program could lead to inconsistent class definitions.
I would think that's why I wrote that custom class, but I don't really remember, to be honest.

@den-run-ai
Copy link
Contributor

@ArvidJB so what do you recommend?

@Cronan
Copy link
Contributor Author

Cronan commented Feb 16, 2018

@ArvidJB @denfromufa ConcurrentQueue would preserve the ordering, and also allow threadsafe snapshot enumeration.

@den-run-ai
Copy link
Contributor

den-run-ai commented Feb 26, 2018

this PR complies with point 1 from these new rules:

#610

@den-run-ai den-run-ai merged commit df2aa64 into pythonnet:master Feb 26, 2018
@Cronan Cronan deleted the lock_recursion branch February 26, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AssemblyList throws LockRecursionException
3 participants