Copilot commented on code in PR #15883:
URL: https://github.com/apache/dubbo/pull/15883#discussion_r2638819086
##########
dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/directory/AbstractDirectory.java:
##########
@@ -208,27 +211,30 @@ 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();
+ }
try {
- if (routerChain != null) {
- routerChain.getLock().readLock().lock();
+ if (!invokerRefreshReadLock.tryLock(LockUtils.DEFAULT_TIMEOUT,
TimeUnit.MILLISECONDS)) {
+ throw new RpcException("Failed to acquire
invokerRefreshLock for reading invokers");
}
// 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("Failed to refresh invokers, cause: " +
e.getMessage(), e);
Review Comment:
The error message "Failed to refresh invokers" is inaccurate in this
context. The code is not refreshing invokers here - it's trying to acquire a
read lock to list/read the invokers. The InterruptedException occurs during
lock acquisition, not during a refresh operation. Consider a more accurate
message such as "Interrupted while acquiring read lock for invoker access".
```suggestion
throw new RpcException("Interrupted while acquiring read
lock for invoker access, cause: " + e.getMessage(), e);
```
##########
dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/directory/AbstractDirectory.java:
##########
@@ -208,27 +211,30 @@ 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();
+ }
try {
- if (routerChain != null) {
- routerChain.getLock().readLock().lock();
+ if (!invokerRefreshReadLock.tryLock(LockUtils.DEFAULT_TIMEOUT,
TimeUnit.MILLISECONDS)) {
+ throw new RpcException("Failed to acquire
invokerRefreshLock for reading invokers");
Review Comment:
The error message "Failed to acquire invokerRefreshLock for reading
invokers" is slightly misleading. It suggests the lock is specifically for
"reading invokers", but this is a read lock on invokerRefreshLock. Consider
clarifying this is about acquiring the read lock, for example: "Failed to
acquire read lock on invokerRefresh within timeout".
```suggestion
throw new RpcException("Failed to acquire read lock on
invokerRefreshLock within timeout");
```
##########
dubbo-cluster/src/main/java/org/apache/dubbo/rpc/cluster/directory/AbstractDirectory.java:
##########
@@ -208,27 +211,30 @@ 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();
+ }
try {
- if (routerChain != null) {
- routerChain.getLock().readLock().lock();
+ if (!invokerRefreshReadLock.tryLock(LockUtils.DEFAULT_TIMEOUT,
TimeUnit.MILLISECONDS)) {
+ throw new RpcException("Failed to acquire
invokerRefreshLock for reading invokers");
}
// 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("Failed to refresh invokers, cause: " +
e.getMessage(), e);
} finally {
- if (routerChain != null) {
- routerChain.getLock().readLock().unlock();
- }
+ invokerRefreshReadLock.unlock();
}
Review Comment:
There is a critical bug: invokerRefreshReadLock.unlock() is called
unconditionally in the finally block, but if tryLock() throws an
InterruptedException, the lock was never acquired. Calling unlock() on a lock
that isn't held will throw IllegalMonitorStateException, which will mask the
original InterruptedException.
The fix should track whether the lock was successfully acquired before
attempting to unlock it. Consider using a boolean flag to track lock
acquisition status.
--
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]
