Copilot commented on code in PR #15883:
URL: https://github.com/apache/dubbo/pull/15883#discussion_r2639705413
##########
dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/directory/AbstractDirectory.java:
##########
@@ -208,27 +211,35 @@ public List<Invoker<T>> list(Invocation invocation)
throws RpcException {
BitList<Invoker<T>> availableInvokers;
SingleRouterChain<T> singleChain = null;
try {
+ if (routerChain != null) {
+ routerChain.getLock().readLock().lock();
+ }
+ boolean lockAcquired = false;
try {
- if (routerChain != null) {
- routerChain.getLock().readLock().lock();
+ if (!invokerRefreshReadLock.tryLock(LockUtils.DEFAULT_TIMEOUT,
TimeUnit.MILLISECONDS)) {
+ throw new RpcException("Failed to acquire read lock on
invokerRefreshLock within timeout");
}
+ lockAcquired = true;
// use clone to avoid being modified at doList().
if (invokersInitialized) {
availableInvokers = validInvokers.clone();
} else {
availableInvokers = invokers.clone();
}
-
- if (routerChain != null) {
- singleChain = routerChain.getSingleChain(getConsumerUrl(),
availableInvokers, invocation);
- singleChain.getLock().readLock().lock();
- }
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ throw new RpcException(
+ "Interrupted while acquiring read lock for invoker
access, cause: " + e.getMessage(), e);
} finally {
- if (routerChain != null) {
- routerChain.getLock().readLock().unlock();
+ if (lockAcquired) {
+ invokerRefreshReadLock.unlock();
}
}
Review Comment:
The conversion from ReentrantLock to ReentrantReadWriteLock is a significant
concurrency change that improves read performance but introduces new complexity
with the read/write lock semantics. This change should be covered by concurrent
access tests that verify:
1. Multiple threads can successfully acquire the read lock simultaneously
during list() calls
2. Write operations properly block read operations when modifying invoker
lists
3. The timeout mechanism works correctly under contention
4. No deadlocks occur when acquiring multiple locks in the nested pattern
used in list()
Consider adding tests that simulate the scenario described in the PR:
concurrent read routes from consumers during service provider release periods.
##########
dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/directory/AbstractDirectory.java:
##########
@@ -208,27 +211,35 @@ public List<Invoker<T>> list(Invocation invocation)
throws RpcException {
BitList<Invoker<T>> availableInvokers;
SingleRouterChain<T> singleChain = null;
try {
+ if (routerChain != null) {
+ routerChain.getLock().readLock().lock();
+ }
+ boolean lockAcquired = false;
try {
- if (routerChain != null) {
- routerChain.getLock().readLock().lock();
+ if (!invokerRefreshReadLock.tryLock(LockUtils.DEFAULT_TIMEOUT,
TimeUnit.MILLISECONDS)) {
+ throw new RpcException("Failed to acquire read lock on
invokerRefreshLock within timeout");
Review Comment:
The error message "Failed to acquire read lock on invokerRefreshLock within
timeout" should include more context to help with debugging. Consider adding
information such as the current lock state, whether it's held by a write lock,
the timeout value used, and the service being accessed. This will make
troubleshooting lock contention issues much easier in production environments.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
