You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "lokidundun (via GitHub)" <gi...@apache.org> on 2025/10/21 14:48:52 UTC
[PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
lokidundun opened a new pull request, #7719:
URL: https://github.com/apache/incubator-seata/pull/7719
…t with ReentrantLock to adapt to virtual thread environment
<!--
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<!-- Please make sure you have read and understood the contributing guidelines -->
- [x] I have read the [CONTRIBUTING.md](https://github.com/apache/incubator-seata/blob/2.x/CONTRIBUTING.md) guidelines.
- [x] I have registered the PR [changes](https://github.com/apache/incubator-seata/tree/2.x/changes).
### Ⅰ. Describe what this PR did
replace synchronization logic in AbstractNettyRemotingClient with ReentrantLock to adapt to virtual thread environment.
### Ⅱ. Does this pull request fix one issue?
<!-- If that, add "fixes #xxx" below in the next line, for example, fixes #97. -->
fixes #7681
### Ⅲ. Why don't you add test cases (unit test/integration test)?
### Ⅳ. Describe how to verify it
### Ⅴ. Special notes for reviews
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "maple525866 (via GitHub)" <gi...@apache.org>.
maple525866 commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2448989833
##########
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:
<img width="550" height="208" alt="image" src="https://github.com/user-attachments/assets/6e855c49-db96-4642-8db6-ebc9a9a5c4b6" />
Before the change, the parent class lock was referenced here
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2451050591
##########
config/seata-config-core/pom.xml:
##########
@@ -43,6 +43,10 @@
<groupId>org.yaml</groupId>
<artifactId>snakeyaml</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.commons</groupId>
+ <artifactId>commons-lang3</artifactId>
+ </dependency>
Review Comment:
The addition of commons-lang3 dependency appears unrelated to the PR's stated purpose of replacing synchronization logic with ReentrantLock. This dependency addition should either be removed from this PR or explained in the PR description.
```suggestion
```
##########
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##########
@@ -90,7 +92,12 @@ public abstract class AbstractNettyRemotingClient extends AbstractNettyRemoting
private final CopyOnWriteArrayList<ChannelEventListener> channelEventListeners = new CopyOnWriteArrayList<>();
- protected final Object mergeLock = new Object();
+ // synchronized(mergeLock)
+ // mergeLock.wait()/notify()/notifyAll()
+ // Mixed use will cause race conditions or IllegalMonitorStateException
Review Comment:
This comment references the old `synchronized(mergeLock)` pattern but should be updated to reflect the new ReentrantLock pattern. Consider revising to something like: 'Use mergeLock.lock()/unlock() with mergeCondition.await()/signalAll() for thread coordination. Do not mix with synchronized blocks.'
```suggestion
// Use mergeLock.lock()/unlock() with mergeCondition.await()/signalAll() for thread coordination.
// Do not mix with synchronized blocks or Object.wait()/notify()/notifyAll(), as this will cause race conditions or IllegalMonitorStateException.
```
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "funky-eyes (via GitHub)" <gi...@apache.org>.
funky-eyes commented on PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#issuecomment-3434843436
Thank you for your contribution. If you have a DingTalk ID, please kindly email it to `jianbin@apache.org` and I will invite you to join the Seata developers' group.
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#issuecomment-3427270840
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `43.75000%` with `9 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 61.80%. Comparing base ([`e240db8`](https://app.codecov.io/gh/apache/incubator-seata/commit/e240db83c3fe18d2dcd473ead672bd959d49e750?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`de1fd53`](https://app.codecov.io/gh/apache/incubator-seata/commit/de1fd53c816abf035a14444cdba9b22d82269971?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ta/core/rpc/netty/AbstractNettyRemotingClient.java](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?src=pr&el=tree&filepath=core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fseata%2Fcore%2Frpc%2Fnetty%2FAbstractNettyRemotingClient.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2VhdGEvY29yZS9ycGMvbmV0dHkvQWJzdHJhY3ROZXR0eVJlbW90aW5nQ2xpZW50LmphdmE=) | 43.75% | [9 Missing :warning: ](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 2.x #7719 +/- ##
============================================
+ Coverage 61.79% 61.80% +0.01%
Complexity 680 680
============================================
Files 1324 1324
Lines 50046 50050 +4
Branches 5910 5910
============================================
+ Hits 30925 30933 +8
+ Misses 16306 16304 -2
+ Partials 2815 2813 -2
```
| [Files with missing lines](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ta/core/rpc/netty/AbstractNettyRemotingClient.java](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?src=pr&el=tree&filepath=core%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fseata%2Fcore%2Frpc%2Fnetty%2FAbstractNettyRemotingClient.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvc2VhdGEvY29yZS9ycGMvbmV0dHkvQWJzdHJhY3ROZXR0eVJlbW90aW5nQ2xpZW50LmphdmE=) | `48.30% <43.75%> (+0.02%)` | :arrow_up: |
... and [4 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-seata/pull/7719/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
[](https://app.codecov.io/gh/apache/incubator-seata/pull/7719?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "maple525866 (via GitHub)" <gi...@apache.org>.
maple525866 commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2448989833
##########
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:
<img width="550" height="208" alt="image" src="https://github.com/user-attachments/assets/6e855c49-db96-4642-8db6-ebc9a9a5c4b6" />
Before the change, the parent class lock was referenced here
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "lokidundun (via GitHub)" <gi...@apache.org>.
lokidundun commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2448952376
##########
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:
Parent AbstractNettyRemoting uses independent locks. mergeLock (for batch sending) is isolated—no overlap with parent’s synchronized logic
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "maple525866 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "lokidundun (via GitHub)" <gi...@apache.org>.
lokidundun commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2450444588
##########
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:
Thanks for telling me.
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "funky-eyes (via GitHub)" <gi...@apache.org>.
funky-eyes merged PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "maple525866 (via GitHub)" <gi...@apache.org>.
maple525866 commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2448765389
##########
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)
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "lokidundun (via GitHub)" <gi...@apache.org>.
lokidundun commented on PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#issuecomment-3435210983
> Thank you for your contribution. If you have a DingTalk ID, please kindly email it to `jianbin@apache.org` and I will invite you to join the Seata developers' group.
Thanks for your notice. I have sent an email to jianbin@apache.org with my DingTalk ID.Looking forward to your invitation. Thank you!
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] refactor: replace synchronization logic in AbstractNettyRemotingClient [incubator-seata]
Posted by "maple525866 (via GitHub)" <gi...@apache.org>.
maple525866 commented on code in PR #7719:
URL: https://github.com/apache/incubator-seata/pull/7719#discussion_r2448856453
##########
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:
Obviously, at least the parent class is still using synchronized
--
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@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org