Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-23 Thread via GitHub 


funky-eyes merged PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-20 Thread via GitHub 


Dltmd202 commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3094424743

   I’ve finished the changes. Would you mind taking a look when you have time?
   @funky-eyes


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-16 Thread via GitHub 


Dltmd202 commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3079209243

   Sure, I’ll take care of it!


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-15 Thread via GitHub 


funky-eyes commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3076505572

18.18% of diff hit (target 60.50%)
   Can you add some test cases to improve the coverage?


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-11 Thread via GitHub 


Dltmd202 commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2201130128


##
core/src/test/java/org/apache/seata/core/rpc/netty/ResourceCleanupTest.java:
##
@@ -143,6 +145,26 @@ void testCleanupWithNullChannel() {
 assertTrue(futures.containsKey(1), "Future ID 1 should still exist");
 }
 
+@Test
+void testExceptionCaughtTriggersChannelRelease() throws Exception {
+AbstractNettyRemotingClient.ClientHandler handler = client.new 
ClientHandler();
+ChannelHandlerContext mockCtx = mock(ChannelHandlerContext.class);
+when(mockCtx.channel()).thenReturn(channel);
+when(channel.remoteAddress()).thenReturn(new 
InetSocketAddress("127.0.0.1", 8091));
+Field channelManagerField = 
AbstractNettyRemotingClient.class.getDeclaredField("clientChannelManager");
+channelManagerField.setAccessible(true);
+NettyClientChannelManager originalManager = 
(NettyClientChannelManager) channelManagerField.get(client);
+
+NettyClientChannelManager spyManager = spy(originalManager);
+channelManagerField.set(client, spyManager);
+
+handler.exceptionCaught(mockCtx, new IllegalArgumentException("test"));
+
+Thread.sleep(500);
+verify(spyManager).releaseChannel(eq(channel), anyString());

Review Comment:
   You’re right — that makes sense to me as well. I’ll go ahead and update it 
to match the exact value instead of using `anyString()`.



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-09 Thread via GitHub 


PeppaO commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3055149453

   After resolving the disconnection and reconnection, perform a stress test to 
see if the TPS is 0 during the shutdown period after killing one of the tc 
nodes.
   @funky-eyes @Dltmd202 


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-09 Thread via GitHub 


PeppaO commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3055146611

   To reproduce, start three tc nodes 8091/8092/8093, start a business-xa 
application, observe the corresponding tcp connection port, then kill 8091, and 
observe whether the tcp connection is disconnected and reconnected


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-09 Thread via GitHub 


PeppaO commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3055143919

   This modification cannot solve the problem. I think you can reproduce the 
problem first, then analyze the cause, and retest after modification.


-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-09 Thread via GitHub 


YongGoose commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2196352884


##
core/src/test/java/org/apache/seata/core/rpc/netty/ResourceCleanupTest.java:
##
@@ -143,6 +145,26 @@ void testCleanupWithNullChannel() {
 assertTrue(futures.containsKey(1), "Future ID 1 should still exist");
 }
 
+@Test
+void testExceptionCaughtTriggersChannelRelease() throws Exception {
+AbstractNettyRemotingClient.ClientHandler handler = client.new 
ClientHandler();
+ChannelHandlerContext mockCtx = mock(ChannelHandlerContext.class);
+when(mockCtx.channel()).thenReturn(channel);
+when(channel.remoteAddress()).thenReturn(new 
InetSocketAddress("127.0.0.1", 8091));
+Field channelManagerField = 
AbstractNettyRemotingClient.class.getDeclaredField("clientChannelManager");
+channelManagerField.setAccessible(true);
+NettyClientChannelManager originalManager = 
(NettyClientChannelManager) channelManagerField.get(client);
+
+NettyClientChannelManager spyManager = spy(originalManager);
+channelManagerField.set(client, spyManager);
+
+handler.exceptionCaught(mockCtx, new IllegalArgumentException("test"));
+
+Thread.sleep(500);
+verify(spyManager).releaseChannel(eq(channel), anyString());

Review Comment:
   Is there a specific reason you used `anyString()`?
   It seems like serverAddress will always be `127.0.0.1:8091`, so I’m 
wondering if matching the exact value would be more appropriate.



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-09 Thread via GitHub 


Dltmd202 commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2196273206


##
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##
@@ -137,6 +140,13 @@ public void init() {
 new NamedThreadFactory(getThreadPrefix(), 
MAX_MERGE_SEND_THREAD));
 mergeSendExecutorService.submit(new MergedSendRunnable());
 }
+reconnectExecutor = new ThreadPoolExecutor(

Review Comment:
   I’ve finished the changes. Would you mind taking a look when you have time?
   @funky-eyes @YongGoose 



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-08 Thread via GitHub 


Dltmd202 commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2193707103


##
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##
@@ -137,6 +140,13 @@ public void init() {
 new NamedThreadFactory(getThreadPrefix(), 
MAX_MERGE_SEND_THREAD));
 mergeSendExecutorService.submit(new MergedSendRunnable());
 }
+reconnectExecutor = new ThreadPoolExecutor(

Review Comment:
   Thanks for pointing that out. I revisited the issue and I now see your point 
more clearly — reusing timeoutExecutor makes sense, especially since it’s 
already used for similar tasks like handling reconnections and timeouts. 
Avoiding an extra thread pool also helps keep things lean and easier to manage.
   
   I’ll go ahead and update the code to reuse timeoutExecutor accordingly. 
Appreciate the feedback!



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-07 Thread via GitHub 


funky-eyes commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2191522622


##
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##
@@ -137,6 +140,13 @@ public void init() {
 new NamedThreadFactory(getThreadPrefix(), 
MAX_MERGE_SEND_THREAD));
 mergeSendExecutorService.submit(new MergedSendRunnable());
 }
+reconnectExecutor = new ThreadPoolExecutor(

Review Comment:
   > Hi @funky-eyes I added a separate thread pool mainly to give it a clear 
thread name, so it’s easier to trace when reconnect-related issues happen. But 
if that feels unnecessary here, I’m happy to switch to using the existing 
`timerExecutor`. Let me know what you think!
   
   I wrote very clearly in the issue why we should reuse the timeoutExecutor. I 
suggest you can take a look at the reason.
   



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-07 Thread via GitHub 


YongGoose commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2191491996


##
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##
@@ -137,6 +140,13 @@ public void init() {
 new NamedThreadFactory(getThreadPrefix(), 
MAX_MERGE_SEND_THREAD));
 mergeSendExecutorService.submit(new MergedSendRunnable());
 }
+reconnectExecutor = new ThreadPoolExecutor(

Review Comment:
   IMO, creating a new thread pool is generally preferred when there’s a 
high-priority task or when real-time processing is critical. Also, how 
frequently the issue occurs can be an important factor in deciding whether to 
create a new thread pool.
   
   From a simple traceability standpoint, the current `timeoutExecutor` is used 
in several places — such as handling reconnections and removing timed-out 
messages.
   
   So, it might make sense to update the prefix to something more general that 
fits well across all these usages, and reuse it accordingly.



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-07 Thread via GitHub 


Dltmd202 commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2191272742


##
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##
@@ -137,6 +140,13 @@ public void init() {
 new NamedThreadFactory(getThreadPrefix(), 
MAX_MERGE_SEND_THREAD));
 mergeSendExecutorService.submit(new MergedSendRunnable());
 }
+reconnectExecutor = new ThreadPoolExecutor(

Review Comment:
   Hi @funky-eyes
   I added a separate thread pool mainly to give it a clear thread name, so 
it’s easier to trace when reconnect-related issues happen. But if that feels 
unnecessary here, I’m happy to switch to using the existing `timerExecutor`. 
Let me know what you think!



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-06 Thread via GitHub 


funky-eyes commented on code in PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#discussion_r2188800114


##
core/src/main/java/org/apache/seata/core/rpc/netty/AbstractNettyRemotingClient.java:
##
@@ -137,6 +140,13 @@ public void init() {
 new NamedThreadFactory(getThreadPrefix(), 
MAX_MERGE_SEND_THREAD));
 mergeSendExecutorService.submit(new MergedSendRunnable());
 }
+reconnectExecutor = new ThreadPoolExecutor(

Review Comment:
   Why not use the timerExecutor thread pool directly?



-- 
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]

Re: [PR] bugfix: prevent Netty I/O thread blocking by async channel release via reconnectExecutor [incubator-seata]

2025-07-06 Thread via GitHub 


codecov[bot] commented on PR #7505:
URL: https://github.com/apache/incubator-seata/pull/7505#issuecomment-3042920696

   ## 
[Codecov](https://app.codecov.io/gh/apache/incubator-seata/pull/7505?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 Report
   Attention: Patch coverage is `45.45455%` with `6 lines` in your changes 
missing coverage. Please review.
   > Project coverage is 60.39%. Comparing base 
[(`d2a18ae`)](https://app.codecov.io/gh/apache/incubator-seata/commit/d2a18aef82c08535e4134642070c39d98654f0f6?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 to head 
[(`44b5691`)](https://app.codecov.io/gh/apache/incubator-seata/commit/44b5691b83a49aae1595d4e80607fae6ad0dd019?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/7505?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/7505?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=)
 | 45.45% | [6 Missing :warning: 
](https://app.codecov.io/gh/apache/incubator-seata/pull/7505?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 |
   
   Additional details and impacted files
   
   
   ```diff
   @@ Coverage Diff  @@
   ##2.x#7505  +/-   ##
   
   + Coverage 60.38%   60.39%   +0.01% 
 Complexity  658  658  
   
 Files  1284 1284  
 Lines 4846548475  +10 
 Branches   5694 5695   +1 
   
   + Hits  2926429276  +12 
   + Misses1658716581   -6 
   - Partials   2614 2618   +4 
   ```
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/incubator-seata/pull/7505?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/7505?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=)
 | `38.01% <45.45%> (+0.39%)` | :arrow_up: |
   
   ... and [8 files with indirect coverage 
changes](https://app.codecov.io/gh/apache/incubator-seata/pull/7505/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   [![Impacted file tree 
graph](https://app.codecov.io/gh/apache/incubator-seata/pull/7505/graphs/tree.svg?width=650&height=150&src=pr&token=tbmHt2ZfxO&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/7505?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   
:rocket: New features to boost your workflow: 
   
   - :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.
   


-- 
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: