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

Changed simple synchronization to ReentrantReadWriteLock #712

Closed
wants to merge 1 commit into from

Conversation

LupusMKW
Copy link

@LupusMKW LupusMKW commented Apr 2, 2024

Due to 4f33be6 our application does no longer work.

The commit improved the situation with respect to concurrent mutability of the Services array. Previously, it was possible to read the array while it was being modified concurrently.
Unfortunately, it can now happen that Tomcat runs into a deadlock if a starting application attempts to gather information from the Services array (which all our applications do).

The proposed change replaces the simple lock with a ReentrantReaderWriter lock, so that readers can read the Services array and only writers are blocked.

LupusMKW referenced this pull request Apr 2, 2024
Found by coverity.
@ChristopherSchultz
Copy link
Contributor

Thank you for this PR, though I was already working on a patch and the unit tests pass, so I'm going to commit my patch instead of yours. I will credit you for the idea in the changelog, however.

I like my patch better than yours primarily because you are placing the call to lock.lock() within the try block, and it should be placed before the try block instead. Also, I use two separate Lock objects instead of calling ReentrantReadWriteLock.writeLock() or .readLock() each time, which makes it easier to read the code to see that the correct lock is being used.

I could just add commits to your PR to make it work, but the work is already done on my end so I'm gonna commit mine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants