You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2026/07/09 15:30:06 UTC
[PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
dlmarion opened a new pull request, #6470:
URL: https://github.com/apache/accumulo/pull/6470
Looking at the non-deterministic failures of SessionBlockVerifyIT I found a deadlock between the ClientContext.close and TabletLocator.getLocator methods when there are unclosed Scanners in the client.
JStack found the deadlock at:
```
Java stack information for the threads listed above:
===================================================
"junit-timeout-thread-4":
at org.apache.accumulo.core.clientImpl.TabletLocator.disable(TabletLocator.java:134)
- waiting to lock <0x000000042dbf08e8> (a java.lang.Class for org.apache.accumulo.core.clientImpl.TabletLocator) at
- org.apache.accumulo.core.clientImpl.TabletLocator$1.disable(TabletLocator.java:245) at
- org.apache.accumulo.core.singletons.SingletonManager.disable(SingletonManager.java:106) at
- org.apache.accumulo.core.singletons.SingletonManager$$Lambda/0x00007fbd94325f88.accept(Unknown Source) at
- java.util.ArrayList.forEach(java.base@21.0.11/ArrayList.java:1596) at
- org.apache.accumulo.core.singletons.SingletonManager.transition(SingletonManager.java:196) at
- org.apache.accumulo.core.singletons.SingletonManager.releaseReservation(SingletonManager.java:145)
- locked <0x000000042dcc4b68> (a java.lang.Class for org.apache.accumulo.core.singletons.SingletonManager) at
- org.apache.accumulo.core.singletons.SingletonReservation.close(SingletonReservation.java:50) at
- org.apache.accumulo.core.clientImpl.ClientContext.close(ClientContext.java:894)
- locked <0x000000042d857668> (a org.apache.accumulo.core.clientImpl.ClientContext) at
- org.apache.accumulo.test.functional.SessionBlockVerifyIT.run(SessionBlockVerifyIT.java:179)
"pool-2-thread-5":
at org.apache.accumulo.core.clientImpl.ClientContext.tableZooHelper(ClientContext.java:626)
- waiting to lock <0x000000042d857668> (a org.apache.accumulo.core.clientImpl.ClientContext) at
- org.apache.accumulo.core.clientImpl.ClientContext.getTableState(ClientContext.java:671) at
- org.apache.accumulo.core.clientImpl.TabletLocator.getLocator(TabletLocator.java:148)
- locked <0x000000042dbf08e8> (a java.lang.Class for org.apache.accumulo.core.clientImpl.TabletLocator) at
- org.apache.accumulo.core.clientImpl.ThriftScanner.scan(ThriftScanner.java:518) at
- org.apache.accumulo.core.clientImpl.ScannerIterator.readBatch(ScannerIterator.java:159)
- locked <0x000000042d863fb8> (a org.apache.accumulo.core.clientImpl.ThriftScanner$ScanState) at
- org.apache.accumulo.core.clientImpl.ScannerIterator.getNextBatch(ScannerIterator.java:177) at
- org.apache.accumulo.core.clientImpl.ScannerIterator.hasNext(ScannerIterator.java:109) at
- org.apache.accumulo.test.functional.SessionBlockVerifyIT.lambda$0(SessionBlockVerifyIT.java:122) at
- org.apache.accumulo.test.functional.SessionBlockVerifyIT$$Lambda/0x00007fbd943acba8.call(Unknown Source)
```
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#issuecomment-4929046826
It looks like there's several other places where ensureOpen() is called in a synchronized block. It probably shouldn't do that at all. Most of those look like they are doing it for the same reason (lazy initialization). In at least one case, an object is initialized once with a parameter, and subsequent calls ignore the parameter. So, it looks like there's issues with lazy initialization throughout this code related to ensureOpen().
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#discussion_r3554044184
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -131,8 +131,8 @@ static synchronized boolean isEnabled() {
}
static synchronized void disable() {
- clearLocators();
enabled = false;
+ clearLocators();
Review Comment:
I'm not sure changing this order matters.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -116,7 +116,7 @@ public boolean equals(LocatorKey lk) {
private static final HashMap<LocatorKey,TabletLocator> locators = new HashMap<>();
private static final HashMap<LocatorKey,OfflineTabletLocatorImpl> offlineLocators =
new HashMap<>();
- private static boolean enabled = true;
+ private static volatile boolean enabled = true;
Review Comment:
+1
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -631,51 +631,63 @@ private synchronized TableZooHelper tableZooHelper() {
}
public TableId getTableId(String tableName) throws TableNotFoundException {
+ ensureOpen();
return tableZooHelper().getTableId(tableName);
}
public TableId _getTableIdDetectNamespaceNotFound(String tableName)
throws NamespaceNotFoundException, TableNotFoundException {
+ ensureOpen();
return tableZooHelper()._getTableIdDetectNamespaceNotFound(tableName);
}
public String getTableName(TableId tableId) throws TableNotFoundException {
+ ensureOpen();
return tableZooHelper().getTableName(tableId);
}
public Map<String,TableId> getTableNameToIdMap() {
+ ensureOpen();
return tableZooHelper().getTableMap().getNameToIdMap();
}
public Map<TableId,String> getTableIdToNameMap() {
+ ensureOpen();
return tableZooHelper().getTableMap().getIdtoNameMap();
}
public boolean tableNodeExists(TableId tableId) {
+ ensureOpen();
return tableZooHelper().tableNodeExists(tableId);
}
public void clearTableListCache() {
+ ensureOpen();
tableZooHelper().clearTableListCache();
}
public String getPrintableTableInfoFromId(TableId tableId) {
+ ensureOpen();
return tableZooHelper().getPrintableTableInfoFromId(tableId);
}
public String getPrintableTableInfoFromName(String tableName) {
+ ensureOpen();
return tableZooHelper().getPrintableTableInfoFromName(tableName);
}
public TableState getTableState(TableId tableId) {
+ ensureOpen();
return tableZooHelper().getTableState(tableId, false);
}
public TableState getTableState(TableId tableId, boolean clearCachedState) {
+ ensureOpen();
return tableZooHelper().getTableState(tableId, clearCachedState);
}
public NamespaceId getNamespaceId(TableId tableId) throws TableNotFoundException {
+ ensureOpen();
Review Comment:
The first thing that tableZooHelper() does is ensureOpen(), so this check is redundant on all the calls that are calling tableZooHelper()
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#issuecomment-4939045795
Full IT build passed
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#discussion_r3554189847
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -116,7 +116,7 @@ public boolean equals(LocatorKey lk) {
private static final HashMap<LocatorKey,TabletLocator> locators = new HashMap<>();
private static final HashMap<LocatorKey,OfflineTabletLocatorImpl> offlineLocators =
new HashMap<>();
- private static boolean enabled = true;
+ private static volatile boolean enabled = true;
Review Comment:
Actually, it turns out this doesn't matter, since all accesses are synchronized on the same object monitor. So this is redundant.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#issuecomment-4939440335
@dlmarion looks good. Looks like a pain to merge to main though. Are you working on that?
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#discussion_r3554138798
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -631,51 +631,63 @@ private synchronized TableZooHelper tableZooHelper() {
}
public TableId getTableId(String tableName) throws TableNotFoundException {
+ ensureOpen();
return tableZooHelper().getTableId(tableName);
}
public TableId _getTableIdDetectNamespaceNotFound(String tableName)
throws NamespaceNotFoundException, TableNotFoundException {
+ ensureOpen();
return tableZooHelper()._getTableIdDetectNamespaceNotFound(tableName);
}
public String getTableName(TableId tableId) throws TableNotFoundException {
+ ensureOpen();
return tableZooHelper().getTableName(tableId);
}
public Map<String,TableId> getTableNameToIdMap() {
+ ensureOpen();
return tableZooHelper().getTableMap().getNameToIdMap();
}
public Map<TableId,String> getTableIdToNameMap() {
+ ensureOpen();
return tableZooHelper().getTableMap().getIdtoNameMap();
}
public boolean tableNodeExists(TableId tableId) {
+ ensureOpen();
return tableZooHelper().tableNodeExists(tableId);
}
public void clearTableListCache() {
+ ensureOpen();
tableZooHelper().clearTableListCache();
}
public String getPrintableTableInfoFromId(TableId tableId) {
+ ensureOpen();
return tableZooHelper().getPrintableTableInfoFromId(tableId);
}
public String getPrintableTableInfoFromName(String tableName) {
+ ensureOpen();
return tableZooHelper().getPrintableTableInfoFromName(tableName);
}
public TableState getTableState(TableId tableId) {
+ ensureOpen();
return tableZooHelper().getTableState(tableId, false);
}
public TableState getTableState(TableId tableId, boolean clearCachedState) {
+ ensureOpen();
return tableZooHelper().getTableState(tableId, clearCachedState);
}
public NamespaceId getNamespaceId(TableId tableId) throws TableNotFoundException {
+ ensureOpen();
Review Comment:
Oh, I didn't realize that this was attempting to check before the synchronized block. However, I think that tableZooHelper doesn't need to synchronize on the same object monitor. It appears to only be synchronized for lazy construction, but synchronizing on any other object monitor would suffice for that. Or, it could be a memoized supplier set up in the constructor, and then that method can have its synchronized keyword removed.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#discussion_r3554194132
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/TabletLocator.java:
##########
@@ -131,8 +131,8 @@ static synchronized boolean isEnabled() {
}
static synchronized void disable() {
- clearLocators();
enabled = false;
+ clearLocators();
Review Comment:
I think this also doesn't matter because all accesses of the enabled field are synchronized on the same object monitor.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6470:
URL: https://github.com/apache/accumulo/pull/6470#issuecomment-4939982710
@ctubbsii - merge to main complete. I think there may be some more opportunities to remove `synchronized` method modifiers in ClientContext in `main`. That can be follow on work.
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #6470:
URL: https://github.com/apache/accumulo/pull/6470
--
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org