-
Notifications
You must be signed in to change notification settings - Fork 90
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix MqttServer Duplicate handler name: idle #101
base: master
Are you sure you want to change the base?
Conversation
When The NetServerOptions set the ' IdleTimeout'. The connection will add the name 'idle' handler before. And the mqttserver will add the same name handlee too.
@dushibaiyu , can you implement it in a way, as it done in the MQTT client, I mean deprecating |
Yes。 it changed. |
*/ | ||
@Override | ||
public int getIdleTimeout() { | ||
return 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why returning 0 here? The method will return zero by default.
And, thinking about it... I think we can also delegate the method call to MqttServerOptions#timeoutOnConnect
@Deprecated | ||
@Override | ||
public MqttServerOptions setIdleTimeout(int idleTimeout) { | ||
super.setIdleTimeout(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why calling the super method here? I think we should not do this.
@@ -252,10 +252,29 @@ public MqttServerOptions setTimeoutOnConnect(int timeoutOnConnect) { | |||
return this; | |||
} | |||
|
|||
/** | |||
* Do the same thing as {@link MqttClientOptions#setKeepAliveTimeSeconds(int)}. Use it instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do the same thing as {@link MqttClientOptions#setTimeoutOnConnect(int)}
. Use it instead.
@@ -154,5 +154,6 @@ public void userEventTriggered(ChannelHandlerContext ctx, Object evt) throws Exc | |||
} | |||
} | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please remove this line? Since it is not related to the PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Yes , I have fixed it. |
/** | ||
* @return timeout on CONNECT before closing connection | ||
*/ | ||
public int timeoutOnConnect() { | ||
return this.timeoutOnConnect; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dushibaiyu , don't forget to remove the line, please.
return setTimeoutOnConnect(idleTimeout); | ||
} | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And please remove one empty line here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
can this get merged? |
When The NetServerOptions set the ' IdleTimeout'. The connection will add the name 'idle' handler before.
And the mqttserver will add the same name handler too.