integrated client side keep alive timeout #129

konradmichael · 2019-03-28T13:51:25Z

This is my implementation for fixing the Issue #123. This time it also includes Unit-Tests

There are three main changes:

Introduction of parameter 'keepAliveTimeout' (default:10) - Code comes from 'vietj's branch
Added IdleStateHandler for closing the connection, if no input is coming within timeout
keepAliveHandler will react on READER_IDLE

to Point3:
If you only send messages with QoS-0 you wont get anything from the broker. This way you cannot identify the state of the server connection. If you need to know if the server is responding you have to include the READER_IDLE.
Consequence of this is, that you usually send two PINGREQ, one for WRITER_IDLE and one for READER_IDLE.

I appreciate any help on this topic

vietj · 2019-03-28T18:45:03Z

keepAliveTimeout is for the server not the client, also all tests have not been ported

vietj · 2019-03-28T18:57:43Z

sorry you just renamed on the server the 1.5 factor variable to keepAliveTimeout which was not clear initially

vietj · 2019-03-28T18:58:24Z

I also did a modification of the MqttServerImpl to support disabling of responses to test the case where the server will not send a ping response in time, can you port this + the test as well to have this case covered ?

konradmichael · 2019-03-29T05:30:24Z

keepAliveTimeout is for the server not the client, also all tests have not been ported

I have no Idea what that means? Can you please be more clear? What tests have to be ported?

sorry you just renamed on the server the 1.5 factor variable to keepAliveTimeout which was not clear initially

Sometimes it is more helpful to use a meaningful variable name, timeout is very general. But if you want it to be reverted, just say so
It is a leftover of some test implementation.

I also did a modification of the MqttServerImpl to support disabling of responses to test the case where the server will not send a ping response in time, can you port this + the test as well to have this case covered ?

Yes you made a modification. But after reviewing it, I have not found the use of it, that's why I did not integrate it. All the option did, was to stop the server-side timeout. It did not disable all keepAlive handling (PINGREQ / PINGRESP). So the option name was also misleading.
Also you see, that the test still worked without that option, so again why would you need that option.

I also tried to make some more tests, but they were full of magic numbers and the outcome would not have been meaningful. E.g. How do you test that your connection does NOT timeout ? :)
Testing is an art ...

konradmichael · 2019-03-29T05:34:41Z

The travis checks are failing because of a blocked port :)
Problem here seems to be the parallel run of multiple builds / tests. It might be a good idea to revert the tests to use dynamic free ports.

    try (ServerSocket socket = new ServerSocket(0)) {
        socket.setReuseAddress(true);
        return socket.getLocalPort();
    }

vietj · 2019-03-29T07:53:04Z

I see what you mean now, indeed it is not necessary, in my test I made the mistake to use a setKeepAliveTimeout larger than setKeepAliveTimeSeconds on the client which required this change.

I'll make a few comments in the PR.

You want to test that the client does not timeout because it receives ping in time from the server ?

vietj · 2019-03-29T07:53:14Z

src/test/java/io/vertx/mqtt/test/server/MqttServerKeepAliveTest.java

+        async.complete();
+      });
+    }));
+    async.await(4000);


this is not needed

ok :) But I would like to keep the timeout.
The last time i was running this test and it failed, it did not stop. I havent tried to find the default timeout.
If you know a better way, feel free to change

it's really not useful here, Vert.x Unit will anyway consider the test is not finished until async.complete() is called even though the test method returns

vietj · 2019-03-29T07:53:27Z

src/test/java/io/vertx/mqtt/test/client/MqttClientKeepAliveTest.java

+        async.complete();
+      });
+    }));
+    async.await(4000);


this is not needed

vietj · 2019-03-29T08:00:08Z

src/test/java/io/vertx/mqtt/test/client/MqttClientKeepAliveTest.java

+    startServer(ctx);
+    Async async = ctx.async();
+    MqttClientOptions options = new MqttClientOptions();
+    options.setKeepAliveTimeSeconds(1);


use a larger value than 1

Currently this will tell the server to disconnect after 1 seconds it receives a ping, as the client sends a ping after 1 second, so the server might try to disconnect after 2 seconds, this can race with the fact that we test the client will close the connection and not the server, which can happen in CI and report false negatives.

I think the value 6 is a good trade off: the client will disconnect after 7 seconds (6 + 1) and the server would disconnect after 9 seconds which gives a 2 seconds delta.

You are right. I usually try to keep these values as tiny as possible to reduce test runtime.

konradmichael · 2019-03-29T10:25:06Z

You want to test that the client does not timeout because it receives ping in time from the server ?

That is a complex test with many magic numbers. From my point of view it is very hard to make such a test that looks reasonable. What interval is long enough to wait, so that this is a real test ...

It is always pretty tough, to test that nothing happens :)

I will update the PR on monday, waiting for some more responses and reviews.

vietj · 2019-06-11T06:56:17Z

@ppatierno can you finish the review and merge for 3.8 / master ?

ppatierno · 2019-06-11T07:33:31Z

src/main/java/io/vertx/mqtt/impl/MqttClientImpl.java

+            case WRITER_IDLE:
+              // send ping or broker will close connection
+              ping();
+              break;


Can we do "fall through" here? Both cases have the same ping() instruction

ppatierno · 2019-06-11T07:33:46Z

src/main/java/io/vertx/mqtt/impl/MqttClientImpl.java

            }
          }
        }
      });
+
+      if(this.options.getKeepAliveTimeout() > 0) {


Space after if

ppatierno · 2019-06-11T07:34:07Z

src/main/java/io/vertx/mqtt/impl/MqttServerConnection.java

@@ -243,11 +243,10 @@ private void handleConnect(MqttConnectMessage msg) {
    if (msg.variableHeader().keepAliveTimeSeconds() != 0) {

      // the server waits for one and a half times the keep alive time period (MQTT spec)
-      int timeout = msg.variableHeader().keepAliveTimeSeconds() +
-        msg.variableHeader().keepAliveTimeSeconds() / 2;
+      int keepAliveTimeout =(int) (msg.variableHeader().keepAliveTimeSeconds() * 1.5);


space after =

ppatierno · 2019-06-11T07:34:53Z

src/test/java/io/vertx/mqtt/test/server/MqttServerKeepAliveTest.java

+import static org.junit.Assert.assertEquals;
+
+/**
+ * MQTT client keep alive tests using a Vert.x MQTT server to accomodate testing.


I guess this comment is wrong, the test is about MQTT server not client.

ppatierno · 2019-06-11T07:35:40Z

@vietj @konradmichael I have left a few comments but after them I think we are good to go.

vietj · 2019-07-14T08:15:02Z

it seems the test did fail in CI

vietj · 2019-08-20T07:08:39Z

tests still don't pass, we should have a CI passing to accept this PR - anybody wanting to contribute please step up and help

vietj · 2020-10-07T11:10:39Z

I'm looking at finishing this PR and it seems that the client (which is visible in MqttClientKeepAliveTest) will send 2 pings simultaneously after 2 seconds because the IdleStateHandler will trigger for read and write. That seems incorrect to me.

vietj · 2020-10-07T11:18:45Z

See #173

integrated client side keep alive timeout

703907e

pmlopes added the to review label Mar 28, 2019

konradmichael mentioned this pull request Mar 28, 2019

Auto keep alive not working #123

Closed

vietj reviewed Mar 29, 2019

View reviewed changes

removed timeouts and fixed Test

d2e74fa

vietj added this to the 3.8.0 milestone Jun 11, 2019

ppatierno requested changes Jun 11, 2019

View reviewed changes

vietj modified the milestones: 3.8.0, 3.8.1 Jul 19, 2019

vietj modified the milestones: 3.8.1, 3.8.2 Aug 23, 2019

vietj modified the milestones: 3.8.2, 3.8.3 Oct 4, 2019

vietj modified the milestones: 3.8.3, 3.8.4 Oct 22, 2019

vietj modified the milestones: 3.9.3, 3.9.4 Sep 17, 2020

vietj closed this Oct 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

integrated client side keep alive timeout #129

integrated client side keep alive timeout #129

konradmichael commented Mar 28, 2019

vietj commented Mar 28, 2019

vietj commented Mar 28, 2019

vietj commented Mar 28, 2019

konradmichael commented Mar 29, 2019 •

edited

Loading

konradmichael commented Mar 29, 2019

vietj commented Mar 29, 2019

vietj Mar 29, 2019

konradmichael Mar 29, 2019

vietj Mar 29, 2019

vietj Mar 29, 2019

vietj Mar 29, 2019 •

edited

Loading

konradmichael Mar 29, 2019

konradmichael commented Mar 29, 2019

vietj commented Jun 11, 2019

ppatierno Jun 11, 2019

ppatierno Jun 11, 2019

ppatierno Jun 11, 2019

ppatierno Jun 11, 2019

ppatierno commented Jun 11, 2019

vietj commented Jul 14, 2019

vietj commented Aug 20, 2019

vietj commented Oct 7, 2020

vietj commented Oct 7, 2020

integrated client side keep alive timeout #129

integrated client side keep alive timeout #129

Conversation

konradmichael commented Mar 28, 2019

vietj commented Mar 28, 2019

vietj commented Mar 28, 2019

vietj commented Mar 28, 2019

konradmichael commented Mar 29, 2019 • edited Loading

konradmichael commented Mar 29, 2019

vietj commented Mar 29, 2019

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

vietj Mar 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konradmichael commented Mar 29, 2019

vietj commented Jun 11, 2019

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

ppatierno commented Jun 11, 2019

vietj commented Jul 14, 2019

vietj commented Aug 20, 2019

vietj commented Oct 7, 2020

vietj commented Oct 7, 2020

konradmichael commented Mar 29, 2019 •

edited

Loading

vietj Mar 29, 2019 •

edited

Loading