Repository navigation
Conversation
This code held a lock for `server.openingTablets` but would attempt to modify `server.unopenedTablets` without holding that lock. Other sections of code that lock on these Sets always try to lock `server.unopenedTablets` first. This section of the code doesn't do that and could result in a dead lock situation.
4c56aa2 to
00c3798
Compare
| .contains(fixedExtent)) { | ||
| throw new IllegalStateException( | ||
| "Fixed split does not overlap " + extent + " " + fixedExtent); | ||
| synchronized (server.unopenedTablets) { |
There was a problem hiding this comment.
I agree that the synchronization for this code block is inconsistent with the other code blocks in this method. However, I'm wondering if some of the other places in this method that synchronize on openingTablets and unopenedTablets are not needed. Both of these objects are synchronized SortedSets, which only need to be used in a synchronized block in a couple of cases according to the javadoc.
For example, there are a couple of places where the following is done, but I don't think it's necessary.
synchronized (server.openingTablets) {
server.openingTablets.remove(extent);
server.openingTablets.notifyAll();
}
I don't think that a call to 'openingTablets.removeorunopenedTablets.add` needs to be done in a synchronized block (according to the javadoc) and I'm not sure that this code block needs to be synchronized at all.
There was a problem hiding this comment.
Yeah, I added this as I was investigating a solution for the spotbugs issues that I spun off into #6587.
it would be ideal for this code to be simplified to avoid this instead.
|
#6621 addresses this too. If that is merged this PR should be closed |
This code held a lock for
server.openingTabletsbut would attempt to modifyserver.unopenedTabletswithout holding that Object's lock.Other sections of code that lock on these SortedSets always try to lock
server.unopenedTabletsfirst. This section of the code doesn't do that and could result in a dead lock situation.The fix was to have this code attempt to grab the unopenedTablets lock first so it follows the same lock pattern as other code.