maple525866 commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2448757981
##########
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##########
@@ -183,8 +186,11 @@ public Object sendSyncRequest(Object msg) throws
TimeoutException {
LOGGER.debug("offer message: {}", rpcMessage.getBody());
}
if (!isSending) {
- synchronized (mergeLock) {
- mergeLock.notifyAll();
+ mergeLock.lock();
+ try {
+ mergeCondition.signalAll();
+ } finally {
+ mergeLock.unlock();
Review Comment:
ReentrantLock + Condition.signalAll() cannot wake up the thread that uses
synchronized(lock) { lock.wait(); }. If only the current changes are made, will
the writing thread be blocked forever?
##########
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##########
@@ -90,7 +92,8 @@ public abstract class AbstractNettyRemotingClient extends
AbstractNettyRemoting
private final CopyOnWriteArrayList<ChannelEventListener>
channelEventListeners = new CopyOnWriteArrayList<>();
- protected final Object mergeLock = new Object();
+ protected final ReentrantLock mergeLock = new ReentrantLock();
Review Comment:
You changed mergeLock from Object to ReentrantLock, and changed it to
lock()/await()/signalAll()/unlock() in two places - this is the right approach,
but you must make sure that the code base has no other residual
synchronized(mergeLock) / mergeLock.wait()/notify() usage. Mixing will lead to
strange race conditions or IllegalMonitorState (calling wait/notify without
holding monitor)
##########
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##########
@@ -578,11 +584,14 @@ private class MergedSendRunnable implements Runnable {
@Override
public void run() {
while (true) {
- synchronized (mergeLock) {
- try {
- mergeLock.wait(MAX_MERGE_SEND_MILLS);
- } catch (InterruptedException e) {
- }
+ mergeLock.lock();
+ try {
+ mergeCondition.await(MAX_MERGE_SEND_MILLS,
TimeUnit.MILLISECONDS);
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ LOGGER.warn("MergedSendRunnable wait interrupted", e);
Review Comment:
This changes the thread's visibility to interrupts. Are there any other
issues?
--
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]
