websockets: Count pings from server as activity for idle_timeout #2718
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2718 +/- ##
===========================================
- Coverage 93.03% 93.03% -0.01%
===========================================
Files 178 178
Lines 13687 13684 -3
===========================================
- Hits 12734 12731 -3
Misses 953 953
Continue to review full report in Codecov by Sentry.
|
|
Can't this be configured already? beast::websocket::stream_base::timeout to;
my_ws.get_option(to);
to.keep_alive_pings = true;
my_ws.set_option(to); |
|
This PR changes the behavior when Without the change: receiving control frames does not count as "activity" With the change: receiving control frames counts as activity I could make that behaviour change another config member of the timeout struct if wanted. |
|
I understand that - but why? Isn't setting the option achieving the same thing? What's the difference? |
|
Ah. The setting makes you send ping frames at an even interval. This change counts incoming control frames as activity regardless of whether or not you are sending pings yourself. |
|
Receiving control frames is not activity. So we would need another option to allow the pings to be counted as such. This all seems off to me, why don't you just send pings or increase the timeout? |
|
I first reported this because I was confused by the If not, timely disconnect detection needs ping and pong to be sent in both directions, doubling the number of packets sent/received for no additional benefit. Not a big issue, but I feel counting ping as activity is the more intuitive behaviour. Adding this as a |
|
Updated the PR with tests, @ashtum I feel using a second wall clock time may be excessive, but seems to be somewhat in line with what other tests have done |
|
Can/should I add/change documentation somehow? Is it sufficient that I update the inline documentation in |
|
Yes, please update the docs here as well (if there is anything related). |
|
@xim Anything I can help with? |
|
Sorry, @ashtum, this just disappeared in the Big Pile of Stuff. I'll try to update docs on Monday |
|
Apologies for forgetting about the documentation update. I rebased on develop, added documentation and pushed again. |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
1 similar comment
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
I'm happy with this now. If you want me to squash all commits, or fixup my fixup, or anything, just give the word =) |
|
This can be squashed on our side, this looks like a single commit. From what I can tell @vinniefalco approves the behavioural change, so everything looks good to me. |
|
Looks like one of the tests has a timer that's 500ms and expects it to have been triggered after 600ms. I'll make it more permissive. |
|
Made that 750ms, effectively bumping the slack from 100ms to 250ms |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
Apologies for the noise. Tweaking these values is hard. |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
1 similar comment
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
@xim Why did you alter the timing values in the tests? Did you encounter any failures as a result of the tight timings? |
|
Yes. I had to increase/change margins many times before all the different CI builders were green at the same time. The original timing worked once across the board, but not on the next run. The current timing should be stable. I'd love to see these tests run with stubbed time, for stability and execution speed. But don't know if that's readily available. |
Yes, but without |
Yeah, my point was that this might have been an unintentional behavior because we wanted to only send the ping message when Besides that, other parts of the code show that we have been counting control frames as activity (by unconditionally setting idle_counter to zero). |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
@xim, could you please squash these commits into a single commit? |
|
Yeah, the timing there is apparently night-impossible to get correct in a way that always passes on all platforms. In that one case, a ping hasn't been processed within 400ms. I can increase everything 25% again so it's 500ms. But is anyone has a way of running these tests with stubbed time, that would be preferable... |
If the stream is receiving control packets like ping, don't count it as idle. This means you can enable `timeout_opt.keep_alive_ping` on only one side to get heartbeat. Make tests that verify current behaviour. Update documentation to match changes in PR boostorg#2718. Addresses issue boostorg#2716
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
The failures appear unrelated to my commit |
test/beast/websocket/ping.cpp
Outdated
| BEAST_EXPECT(ws.impl_->idle_counter == 0); | ||
|
|
||
| test::run_for(ioc_, std::chrono::milliseconds(1250)); | ||
| // After 1.25s idle, no timeout but idle counter is 1 | ||
| BEAST_EXPECT(ws.impl_->idle_counter == 1); | ||
|
|
||
| es.async_ping(); | ||
| test::run_for(ioc_, std::chrono::milliseconds(500)); | ||
| // The server sent a ping at 1.25s mark, and we're now at 1.75s mark. | ||
| // We haven't hit the idle timer yet (happens at 1s, 2s, 3s) | ||
| BEAST_EXPECT(ws.impl_->idle_counter == 0); | ||
| BEAST_EXPECT(!got_timeout); | ||
|
|
||
| test::run_for(ioc_, std::chrono::milliseconds(750)); | ||
| // At 2.5s total; should have triggered the idle timer | ||
| BEAST_EXPECT(ws.impl_->idle_counter == 1); |
There was a problem hiding this comment.
I believe the tight timing is a consequence of asserting idle_counter in multiple places. There is a good chance that these timings could lead to non-deterministic test results. Therefore, I suggest not checking for idle_counter and instead only verifying the side effect (inactivity timeout doesn't occur when receiving pings).
If the stream is receiving control packets like ping, don't count it as idle. This means you can enable `timeout_opt.keep_alive_ping` on only one side to get heartbeat. Make tests that verify current behaviour. Update documentation to match changes in PR boostorg#2718. Addresses issue boostorg#2716
|
@xim, I've removed the |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
ashtum
force-pushed
the
develop
branch
2 times, most recently
from
3491313
to
14a1550
Compare
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
|
An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html |
| @@ -227,8 +227,6 @@ class stream<NextLayer, deflateSupported>::read_some_op | |||
| // Handle ping frame | |||
| if(impl.rd_fh.op == detail::opcode::ping) | |||
| { | |||
| impl.update_timer(this->get_executor()); | |||
There was a problem hiding this comment.
Updating the timer is necessary; otherwise, it will attempt to ping the other side even after receiving a ping.
If the stream is receiving control packets like ping, don't count it as idle. This means you can enable
timeout_opt.keep_alive_pingon only one side to get heartbeat.Addresses issue #2716