fix the bug: the channel will not closed in time when timeout by using telnet in QOS #7282

wathenjiang · 2021-02-27T11:20:28Z

What is the purpose of the change

Fix the bug: don't close the channel when timeout by using telnet in QOS.

Brief changelog

When using IdleStateHandler with TelnetProcessHandler in the QosProcessHandler, the timeout is set to 5*60 seconds, but NO handler deals with the IdleStateEvent. Maybe the TelnetProcessHandler should override the userEventTriggered method to handle this Event, and close the idle connect.

…QOS.

horizonzy

Create a new handler to process this case will be better.

wathenjiang · 2021-02-27T15:08:46Z

@horizonzy
Thnx.

In my opinion, for the reason of uniform code style, I prefer to add the userEventTriggered method in TelnetProcessHandler. Refer to class NettyClientHandler & NettyServerHandler.

Could you tell me the reason why you think adding a Handler would be better in this case.

horizonzy · 2021-02-27T15:22:46Z

Single responsibility, TelnetProcessHandler just handle the command from client.
In this pr, it's supply a mechanism to close the channel which didn't exchange data for a long time.
In the future, if need add some logic to handle the timeout channel, the logic shouldn't append in TelnetProcessHandler.
And if anyone didn't need this mechanism, just didn't add the new handler

wathenjiang · 2021-02-27T15:28:13Z

Single responsibility, TelnetProcessHandler just handle the command from client.
In this pr, it's supply a mechanism to close the channel which didn't exchange data for a long time.
In the future, if need add some logic to handle the timeout channel, the logic shouldn't append in TelnetProcessHandler.
And if anyone didn't need this mechanism, just didn't add the new handler

Yeah, I agree with you, but the fact is that NettyClientHandler & NettyServerHandler don't look like meeting the rule of Single responsibility. I thought it as a code style.. QAQ

horizonzy · 2021-02-27T15:33:53Z

Single responsibility, TelnetProcessHandler just handle the command from client.
In this pr, it's supply a mechanism to close the channel which didn't exchange data for a long time.
In the future, if need add some logic to handle the timeout channel, the logic shouldn't append in TelnetProcessHandler.
And if anyone didn't need this mechanism, just didn't add the new handler

Yeah, I agree with you, but the fact is that NettyClientHandler & NettyServerHandler don't look like meeting the rule of Single responsibility. I thought it as a code style.. QAQ

Hi, I'm not agree with you. NettyClientHandler and NettyServerHandler all didn't coupling with business. But TelnetProcessHandler is for business.

wathenjiang · 2021-02-27T15:40:33Z

Hi, I'm not agree with you. NettyClientHandler and NettyServerHandler all didn't coupling with business. But TelnetProcessHandler is for business.

You are right and enlighten me a lot, now I can see the difference. It is better to use a New Handler.

Thnks a lot, have a good night !

horizonzy · 2021-02-27T15:55:52Z

Hi, apache license should append at the top of the new class.

wathenjiang · 2021-02-27T15:58:39Z

Hi, apache license should append at the top of the new class.

I forgot the commit specifications, plz forgive me. QAQ...

horizonzy · 2021-02-27T16:05:55Z

dubbo-plugin/dubbo-qos/src/main/java/org/apache/dubbo/qos/server/handler/IdleEventHandler.java

+import org.apache.dubbo.common.logger.Logger;
+import org.apache.dubbo.common.logger.LoggerFactory;
+
+@ChannelHandler.Sharable


It's not necessary.

if you means the IdleEventHandler is not necessary, then the p.addLast(new IdleStateHandler(0, 0, 5 * 60)); in QosProcessHandler may be also not necessary.

it is kind of chinglish? QAQ hh

I mean the annotation @Sharable.

It's alway create new TelnetIdleEventHandler for channel.

Ok, you are right.

horizonzy · 2021-02-27T16:09:18Z

dubbo-plugin/dubbo-qos/src/main/java/org/apache/dubbo/qos/server/handler/IdleEventHandler.java

+import org.apache.dubbo.common.logger.LoggerFactory;
+
+@ChannelHandler.Sharable
+public class IdleEventHandler extends ChannelDuplexHandler {


The class name use TelnetIdleEventHandler will be better, what do you think about it.

It is ok. Because when we use HTTP, All HTTP request is HTTP short connection, IdleEventHandler only used for telnet.

codecov-io · 2021-02-27T16:18:47Z

Codecov Report

Merging #7282 (6c38249) into master (4ad2634) will decrease coverage by 0.23%.
The diff coverage is 30.00%.

@@             Coverage Diff              @@
##             master    #7282      +/-   ##
============================================
- Coverage     59.03%   58.80%   -0.24%     
  Complexity      461      461              
============================================
  Files          1043     1044       +1     
  Lines         42445    42411      -34     
  Branches       6222     6191      -31     
============================================
- Hits          25057    24938     -119     
- Misses        14587    14661      +74     
- Partials       2801     2812      +11

Impacted Files	Coverage Δ	Complexity Δ
...che/dubbo/qos/server/handler/IdleEventHandler.java	`22.22% <22.22%> (ø)`	`0.00 <0.00> (?)`
...he/dubbo/qos/server/handler/QosProcessHandler.java	`57.14% <100.00%> (+1.26%)`	`0.00 <0.00> (ø)`
...va/org/apache/dubbo/remoting/TimeoutException.java	`0.00% <0.00%> (-33.34%)`	`0.00% <0.00%> (ø%)`
...ng/transport/dispatcher/all/AllChannelHandler.java	`62.06% <0.00%> (-27.59%)`	`0.00% <0.00%> (ø%)`
...bo/config/event/listener/LoggingEventListener.java	`37.50% <0.00%> (-25.00%)`	`0.00% <0.00%> (ø%)`
...bbo/common/bytecode/CustomizedLoaderClassPath.java	`28.00% <0.00%> (-16.00%)`	`0.00% <0.00%> (ø%)`
...va/org/apache/dubbo/remoting/exchange/Request.java	`72.54% <0.00%> (-15.69%)`	`0.00% <0.00%> (ø%)`
...rg/apache/dubbo/remoting/utils/PayloadDropper.java	`76.92% <0.00%> (-15.39%)`	`0.00% <0.00%> (ø%)`
...ng/transport/dispatcher/WrappedChannelHandler.java	`47.82% <0.00%> (-15.22%)`	`0.00% <0.00%> (ø%)`
.../remoting/transport/netty4/NettyServerHandler.java	`56.81% <0.00%> (-13.64%)`	`0.00% <0.00%> (ø%)`
... and 45 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ad2634...07c4830. Read the comment docs.

horizonzy

LGTM

AlbumenJ

LGTM.

fix the bug: don't close the channel when timeout by using telnet in …

9123ec5

…QOS.

wathenjiang changed the title ~~fix the bug: don't close the channel when timeout by using telnet in QOS~~ fix the bug: the channel will not closed in time when timeout by using telnet in QOS Feb 27, 2021

horizonzy reviewed Feb 27, 2021

View reviewed changes

add new IdleEventHandler for Telnet Communication in QoS

2803e59

add apache license

6c38249

horizonzy reviewed Feb 27, 2021

View reviewed changes

change the class name

07c4830

remove the sharable annotation

a039fc9

horizonzy approved these changes Feb 27, 2021

View reviewed changes

AlbumenJ approved these changes Feb 28, 2021

View reviewed changes

AlbumenJ merged commit 51d5a88 into apache:master Feb 28, 2021

AlbumenJ mentioned this pull request Feb 28, 2021

When using Telnet in QoS, the idle connect may not be closed in time, despite using the idle check. #7281

Closed

2 tasks

AlbumenJ added a commit to AlbumenJ/dubbo that referenced this pull request May 27, 2021

Pick from apache#7282 51d5a88

7b0a664

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix the bug: the channel will not closed in time when timeout by using telnet in QOS #7282

fix the bug: the channel will not closed in time when timeout by using telnet in QOS #7282

wathenjiang commented Feb 27, 2021

horizonzy left a comment

wathenjiang commented Feb 27, 2021

horizonzy commented Feb 27, 2021

wathenjiang commented Feb 27, 2021

horizonzy commented Feb 27, 2021

wathenjiang commented Feb 27, 2021

horizonzy commented Feb 27, 2021

wathenjiang commented Feb 27, 2021

horizonzy Feb 27, 2021

wathenjiang Feb 27, 2021

wathenjiang Feb 27, 2021

horizonzy Feb 27, 2021

wathenjiang Feb 27, 2021

horizonzy Feb 27, 2021

wathenjiang Feb 27, 2021

horizonzy Feb 27, 2021

wathenjiang Feb 27, 2021

codecov-io commented Feb 27, 2021

horizonzy left a comment

AlbumenJ left a comment

fix the bug: the channel will not closed in time when timeout by using telnet in QOS #7282

fix the bug: the channel will not closed in time when timeout by using telnet in QOS #7282

Conversation

wathenjiang commented Feb 27, 2021

What is the purpose of the change

Brief changelog

horizonzy left a comment

Choose a reason for hiding this comment

wathenjiang commented Feb 27, 2021

horizonzy commented Feb 27, 2021

wathenjiang commented Feb 27, 2021

horizonzy commented Feb 27, 2021

wathenjiang commented Feb 27, 2021

horizonzy commented Feb 27, 2021

wathenjiang commented Feb 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Feb 27, 2021

Codecov Report

horizonzy left a comment

Choose a reason for hiding this comment

AlbumenJ left a comment

Choose a reason for hiding this comment