websockets: Count pings from server as activity for idle_timeout

[xim]

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.

Addresses issue #2716

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (e42cf8a) 93.03% compared to head (c8ffce2) 93.03%.

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              

Files	Coverage Δ
include/boost/beast/websocket/impl/stream_impl.hpp	96.16% <100.00%> (-0.03%)	:arrow_down:
include/boost/beast/websocket/stream_base.hpp	26.31% <ø> (ø)

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update e42cf8a...c8ffce2. Read the comment docs.

[klemens-morgenstern]

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

[xim]

This PR changes the behavior when keep_alive_ping is false.

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.

[klemens-morgenstern]

I understand that - but why? Isn't setting the option achieving the same thing? What's the difference?

[xim]

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.

[klemens-morgenstern]

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?

[xim]

I first reported this because I was confused by the idle_timeout / keep_alive_pings config options; Since a pong is activity when you've sent a ping, shouldn't a ping be activity as well?

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 bool incoming_ping_keepalives = false option also makes sense if keeping the old behaviour as default is a priority.

[xim]

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

[xim]

Can/should I add/change documentation somehow? Is it sufficient that I update the inline documentation in stream_base.hpp?

[ashtum]

Yes, please update the docs here as well (if there is anything related).

[ashtum]

@xim Anything I can help with?

[xim]

Sorry, @ashtum, this just disappeared in the Big Pile of Stuff. I'll try to update docs on Monday

[xim]

Apologies for forgetting about the documentation update. I rebased on develop, added documentation and pushed again.

[cppalliance-bot]

An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html

[xim]

I'm happy with this now. If you want me to squash all commits, or fixup my fixup, or anything, just give the word =)

[klemens-morgenstern]

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.

[xim]

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.

[xim]

Made that 750ms, effectively bumping the slack from 100ms to 250ms

[cppalliance-bot]

An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html

[xim]

Apologies for the noise. Tweaking these values is hard.

[cppalliance-bot]

An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://2718.beastdocs.prtest.cppalliance.org/libs/beast/doc/html/index.html

[ashtum]

@xim Why did you alter the timing values in the tests? Did you encounter any failures as a result of the tight timings?

[xim]

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.

[ashtum]

Hey @xim, I wanted to let you know that I've decided to review and merge this after the 1.84 release. I reckon it might be risky to do it for 1.84.

[xim]

I have, for now, implemented most of the desired behaviour on top of the control callback. Looking forward to cleaning up that code ^_^

[vinniefalco]

Why shouldn't control frames count as activity?

[ashtum]

Why shouldn't control frames count as activity?

Did you mean, "Why should control frames count as activity?" If so, we had a discussion in https://github.com/boostorg/beast/issues/2716. The idea is that receiving any form of frame from the other side can be an indication that it is still active.