Skip to content
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

Fixing #308 #312

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Conversation

Shazwazza
Copy link
Contributor

This is a WIP to fix #308

The first part is that this fixes a deadlock issues with MockDirectoryWrapper.

  • This was locking on this which seems common from the java port. This wasn't the problem but lock (this) shouldn't be used since there is no way of knowing if something else is locking on that object which will cause deadlocks. This has been changed to lock on a private field. I have verified that there is no other code that ever locks on a directory object so there was nothing important about locking on this.
  • Deadlocks are easily produced when there are recursive locks which happened often in MockDirectoryWrapper. Take for example the call to MockDirectoryWrapper.CreateOutput which takes a lock, then it calls into MaybeThrowDeterministicException which takes the same lock and then it calls into MaybeThrowIOExceptionOnOpen which also takes the same lock, then Init with the same lock and then within the same method is taking a recursive lock on the same lock, then in AddFileHandle again the same lock. This all occurs within the same method CreateOutput. Meanwhile, if any other thread calls any other method that uses this lock (which seems like all of them) you'll get a deadlock
  • This can be fixed by ensuring there's never recursive locks taken. Currently this means there are private methods suffixed with the term "Locked" which double check they are being called within a lock. The original methods that explicitly create the lock now call into their sister "Locked" methods.

There's currently a temp test in this code which allows to run the slow running test over and over until it runs for longer than the specified time. With the deadlock fixed I think i can now see where the slowness of the method comes from so will now look into profiling this.

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.

Investigate slow test: Lucene.Net.Tests.Index.TestAddIndexes::TestAddIndexesWithCloseNoWait()
2 participants