NightOwl888 opened a new pull request, #940: URL: https://github.com/apache/lucenenet/pull/940
<!-- Thank you for submitting a pull request to our repo. --> <!-- Please do NOT submit PRs for features in newer versions of Lucene. We should keep the target version consistent across our repository to make the upgrade process to newer versions of Lucene go smoothly. --> <!-- If this is your first PR in the Lucene.NET repo, please run through the checklist below to ensure a smooth review and merge process for your PR. --> - [x] You've read the [Contributor Guide](https://github.com/apache/lucenenet/blob/main/CONTRIBUTING.md) and [Code of Conduct](https://www.apache.org/foundation/policies/conduct.html). - [x] You've included unit or integration tests for your change, where applicable. - [x] You've included inline docs for your change, where applicable. - [x] There's an open issue for the PR that you are making. If you'd like to propose a change, please [open an issue](https://github.com/apache/lucenenet/issues/new/choose) to discuss the change or find an existing issue. <!-- Once all that is done, you're ready to go. Open the PR with the content below. --> Summary of the changes (Less than 80 chars) Lucene.Net.Support.Threading.ReentrantLock: Fixed the implementation so it prioritizes new threads obtaining the lock over waiting threads. Fixes #935. Closes #886. ## Description This has been a known issue for some time, however as we got the `DocumentsWriter` working reliably on a single thread most users have not worried about it until it was reported in #935. There were 2 issues that were causing test failures explained in https://github.com/apache/lucenenet/issues/935#issuecomment-2118228819. The second issue turned out to be much more involved to work out how to address even though the most recent solution is actually very simple and lightweight. Instead of calling `UninterruptableMonitor.Enter()` in the `Lock()` method, we call `UninterruptableMonitor.TryEnter()` in a loop that executes `Thread.Yield()`. This allows other threads to acquire the lock even though there are waiting threads. Granted, while this approach seems to reliably pass the tests, it may be a bit naïve of an implementation. While it doesn't peg my CPU and seems to run well on Azure DevOps, I am not sure whether this is the appropriate solution for production scenarios. Suggestions are welcome as to how to improve this. Do note there were 2 prior attempts: - https://github.com/apache/lucenenet/commit/6f2e1290ac067299c6e9b216649a852ade0a07f8 - Uses a timeout to ensure the lock is acquired, but it still has to wait until the queue schedules it until it runs. - https://github.com/apache/lucenenet/commit/89b01e625116afd5ccba9457271a36524dc75ca1 - Uses `ManualResetEventSlim` and a `Queue<T>` to manage the waiting threads. This is a more complete implementation and even passed many of the Apache Harmony tests, but it comes at a pretty steep performance cost. Maybe there is a way to improve this, though. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@lucenenet.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
