Skip to content
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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dushibaiyu
Copy link

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.

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.
@Sammers21
Copy link
Contributor

Sammers21 commented Jun 14, 2018

@dushibaiyu , can you implement it in a way, as it done in the MQTT client, I mean deprecating MqttServerOptions#setIdleTimeout and delegating it to the MqttServerOptions#setTimeoutOnConnect call.

@dushibaiyu
Copy link
Author

Yes。 it changed.

*/
@Override
public int getIdleTimeout() {
return 0;
Copy link
Contributor

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);
Copy link
Contributor

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.
Copy link
Contributor

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
}
}
});

Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@dushibaiyu
Copy link
Author

Yes , I have fixed it.

/**
* @return timeout on CONNECT before closing connection
*/
public int timeoutOnConnect() {
return this.timeoutOnConnect;
}

Copy link
Contributor

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?

Copy link
Contributor

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);
}


Copy link
Contributor

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@hilbert-ralf
Copy link

can this get merged?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants