Pushoff keep alive on ack received #314

xiazhvera · 2023-08-08T15:57:32Z

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

bretambrose

The write timestamp can't be shared/singleton.

If I send 5 publishes at times 1, 2, 3, 4, 5. And then an ack comes back for the first one, the push out needs to be based on the first one's write stamp (1).

xiazhvera · 2023-08-08T17:17:42Z

The write timestamp can't be shared/singleton.

If I send 5 publishes at times 1, 2, 3, 4, 5. And then an ack comes back for the first one, the push out needs to be based on the first one's write stamp (1).

I dont think it matters, we only need to push off based on the "last" write time since that's when our "last transmission" happened.

bretambrose · 2023-08-08T17:21:45Z

The write timestamp can't be shared/singleton.
If I send 5 publishes at times 1, 2, 3, 4, 5. And then an ack comes back for the first one, the push out needs to be based on the first one's write stamp (1).

I dont think it matters, we only need to push off based on the "last" write time since that's when our "last transmission" happened.

It absolutely matters. If the connect is dropped at time 1.5 then the ping cannot be pushed out farther than that. Connection drops aren't instantaneous.

What we're trying to express is the following:

"I know the connection was valid at time T because the request I sent at time T received a response from the server."

Edit: Adding the followup discussion for completeness/documentation:

Consider this situation
Keep alive is 60 time units.
The connection path goes from a very slow network (the device in the field) before eventually reaching a fast network (internet backbone). It takes 20 time units total for a packet to go from one endpoint to another.
So a publish on time 1 arrives at the other end on time 21.
A response sent at time 21 arrives at time 41.
Now simulate what happens when the client sends a steady stream of publishes, one every time unit: publish(1), publish(2), publish(3), etc...
The client receives the ack for publish(1) at time 41. The connection could potentially have been dropped at a point anywhere after time 21 and the ack still make it back to the client. A connection drop on the fast part of the network after the ack squeaks through is easily possible and using 41 as the push-out time is not what we want to do.

codecov-commenter · 2023-08-08T23:22:59Z

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.02% ⚠️

Comparison is base (b777be4) 82.22% compared to head (d962b9c) 82.20%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #314      +/-   ##
==========================================
- Coverage   82.22%   82.20%   -0.02%     
==========================================
  Files          20       20              
  Lines        8670     8678       +8     
==========================================
+ Hits         7129     7134       +5     
- Misses       1541     1544       +3

Files Changed	Coverage Δ
source/client_channel_handler.c	`74.60% <100.00%> (+0.93%)`	⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bretambrose · 2023-08-09T17:08:46Z

tests/v3/connection_state_test.c

@@ -3366,8 +3366,8 @@ AWS_TEST_CASE_FIXTURE(
    &test_data)

 /**
- * Makes a CONNECT, with 1 second keep alive ping interval, send a publish roughly every second, and then ensure NO
- * pings were sent
+ * Makes a CONNECT, with 1 second keep alive ping interval, send a different operation every second, and then ensure NO


If this comment is accurate then I feel like this test is likely overly sensitive to timing conditions and containerization.

bretambrose · 2023-08-09T17:11:26Z

tests/v3/connection_state_test.c

+/**
+ * Makes a CONNECT, with 1 second keep alive ping interval, disable the server auto ack so that we never received ack
+ * back. Send a qos1 publish roughly every second for 4 seconds. As we never received the ACK, we should send a total of
+ * 4 ping


This is alarming. Given a keep aliveof 1 second and a ping timeout of 100ms we should be seeing multiple disconnections which are going to disrupt the logical simplicity of the test.

bretambrose · 2023-08-09T17:18:17Z

tests/v3/connection_state_test.c

+     * We would like to wait for a total of ~4.5 seconds to account for slight drift/jitter.
+     * We have been waiting for 0.8*4=3.2 sec. Wait for another 1 sec here.
+     */
+    aws_thread_current_sleep(1000000000);


I don't see any asserts in this test actually checking anything meaningful.

xiazhvera · 2023-08-09T21:15:34Z

Updated the unit tests.
Also checkout s_test_mqtt_connection_ping_basic_scenario_fn for existing keep alive tests.

bretambrose

Fix comment and ship

bretambrose · 2023-08-10T15:03:08Z

tests/v3/connection_state_test.c

+
+    // Make sure we publish for 4 seconds;
+    while (elapsed_time < test_duration) {
+        /* Publish qos1*/


xiazhvera added 2 commits August 7, 2023 18:13

pushoff keep alive onack

5699c1c

make sure sync data is incritical section

e692847

bretambrose requested changes Aug 8, 2023

View reviewed changes

set request send time for each request

1233ab2

xiazhvera requested a review from bretambrose August 8, 2023 23:19

bretambrose reviewed Aug 9, 2023

View reviewed changes

xiazhvera added 2 commits August 9, 2023 13:38

updat tests

1fb786a

cast timestamp number

cac22ab

xiazhvera requested a review from bretambrose August 9, 2023 21:15

sleep to avoid jitter

d962b9c

bretambrose approved these changes Aug 10, 2023

View reviewed changes

fix comments

3236a0a

xiazhvera merged commit b7a323d into main Aug 10, 2023

xiazhvera deleted the keep_alive_improve branch August 10, 2023 17:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pushoff keep alive on ack received #314

Pushoff keep alive on ack received #314

xiazhvera commented Aug 8, 2023

bretambrose left a comment

xiazhvera commented Aug 8, 2023

bretambrose commented Aug 8, 2023 •

edited

Loading

codecov-commenter commented Aug 8, 2023 •

edited

Loading

bretambrose Aug 9, 2023

bretambrose Aug 9, 2023

bretambrose Aug 9, 2023

xiazhvera commented Aug 9, 2023 •

edited

Loading

bretambrose left a comment

bretambrose Aug 10, 2023

Pushoff keep alive on ack received #314

Pushoff keep alive on ack received #314

Conversation

xiazhvera commented Aug 8, 2023

bretambrose left a comment

Choose a reason for hiding this comment

xiazhvera commented Aug 8, 2023

bretambrose commented Aug 8, 2023 • edited Loading

codecov-commenter commented Aug 8, 2023 • edited Loading

Codecov Report

bretambrose Aug 9, 2023

Choose a reason for hiding this comment

bretambrose Aug 9, 2023

Choose a reason for hiding this comment

bretambrose Aug 9, 2023

Choose a reason for hiding this comment

xiazhvera commented Aug 9, 2023 • edited Loading

bretambrose left a comment

Choose a reason for hiding this comment

bretambrose Aug 10, 2023

Choose a reason for hiding this comment

bretambrose commented Aug 8, 2023 •

edited

Loading

codecov-commenter commented Aug 8, 2023 •

edited

Loading

xiazhvera commented Aug 9, 2023 •

edited

Loading