fix: Fix deathlock during statically registering services#4657
Draft
pavel-jares-bcm wants to merge 1 commit into
Draft
fix: Fix deathlock during statically registering services#4657pavel-jares-bcm wants to merge 1 commit into
pavel-jares-bcm wants to merge 1 commit into
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
When service is statically registered (since #4051) there is one additional lock in the class
ApimlInstanceRegistry(see the synchronization removal). In case of multiple threads it could lead to service freezing on start-up during reading the static definition file.Lock order of registerStatically:
Lock order of register:
Deathlock:
thead1 (register): read.lock()
thead2 (registerStatically): synchronized (lock)
thread1 (register): is waiting for synchronized block to be available (to be done by thread2)
thread2 (registerStatically): is waiting for read.unlock() (to be issued by thread1)
Note: locks are used also by other methods but it should be the issue.
The source is the REST call from any south-bound service during the initialization of static services. The risk is higher when the static definition file or amount of service is bigger.
The additional lock (see the synchronized (lock) in registerStatically) was added because the original method register shouldn't change the property
expectedNumberOfClientsSendingRenews. Using the same lock should prevent exclusivity to update the field. It is not correct solution because of orders of locks.The new solution store information about the change (correction - the oposite operation) in a ThreadLocal, so the locks are not necessary now.
The similar issue was in cancel method as well. But it is not big deal as this method is used very rarelly.
Other changes in the PR becomes from analysis that using reflection is not needed at this version of Eureka (there were very probably changes in the overriden code). We can call the whole method now and it is not necessary to reimplement it via calling private methods via reflection.
Linked to # (issue)
Part of the # (epic)
Type of change
Please delete options that are not relevant.
Checklist:
For more details about how should the code look like read the Contributing guideline