Reduce deadlock probability of ClientContext.close / TabletLocator.getLocator

[dlmarion]

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)

[ctubbsii]

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(). Using separate object monitors for each initialization is a lot of bloat, but a common object monitor to synchronize on for lazy initialization for each may cause another deadlock issue if their initialization uses this (ClientContext) to call into another lazily-initialized method.

It's probably better to use Suppliers.memoize() and to create these suppliers in the constructor to initialize them on first use in their getter when supplier.get() is called. That way, they can be initialized independently without deadlock. The only issue with this is that spotbugs might complain about "this" leaking from a constructor inside the suppliers' lambdas. But those are false positives, since we know "this" isn't actually going to get accessed until the supplier is called the first time.

[dlmarion]

Full IT build passed

[ctubbsii]

@dlmarion looks good. Looks like a pain to merge to main though. Are you working on that?

[dlmarion]

@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.