From 46f80ffa8e3b248a66960297b6951538b56097ce Mon Sep 17 00:00:00 2001 From: Morten Minde Neergaard <169057+xim@users.noreply.github.com> Date: Fri, 11 Aug 2023 14:17:11 +0200 Subject: [PATCH] websockets: Count pings from server as activity for idle_timeout 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 #2718. Addresses issue #2716 --- doc/qbk/06_websocket/06_timeouts.qbk | 4 +- .../beast/websocket/impl/stream_impl.hpp | 12 +-- include/boost/beast/websocket/stream_base.hpp | 6 +- test/beast/websocket/ping.cpp | 73 +++++++++++++++++++ test/beast/websocket/test.hpp | 6 ++ 5 files changed, 89 insertions(+), 12 deletions(-) diff --git a/doc/qbk/06_websocket/06_timeouts.qbk b/doc/qbk/06_websocket/06_timeouts.qbk index cc27dbc61d..e39a5df36b 100644 --- a/doc/qbk/06_websocket/06_timeouts.qbk +++ b/doc/qbk/06_websocket/06_timeouts.qbk @@ -59,8 +59,8 @@ There are three timeout settings which may be set independently on the stream: [[link beast.ref.boost__beast__websocket__stream_base__timeout.idle_timeout `timeout::idle_timeout`]] [`duration`] [ - If no data is received from the peer for a time equal to the idle - timeout, then the connection will time out. + If no data or control frames are received from the peer for a time + equal to the idle timeout, then the connection will time out. The value returned by [link beast.ref.boost__beast__websocket__stream_base.none `stream_base::none()`] may be assigned to disable this timeout. diff --git a/include/boost/beast/websocket/impl/stream_impl.hpp b/include/boost/beast/websocket/impl/stream_impl.hpp index 615b4beb19..1ee3809aa4 100644 --- a/include/boost/beast/websocket/impl/stream_impl.hpp +++ b/include/boost/beast/websocket/impl/stream_impl.hpp @@ -439,12 +439,8 @@ struct stream::impl_type if(timeout_opt.idle_timeout != none()) { idle_counter = 0; - if(timeout_opt.keep_alive_pings) - timer.expires_after( - timeout_opt.idle_timeout / 2); - else - timer.expires_after( - timeout_opt.idle_timeout); + timer.expires_after( + timeout_opt.idle_timeout / 2); BOOST_ASIO_HANDLER_LOCATION(( __FILE__, __LINE__, @@ -569,9 +565,9 @@ struct stream::impl_type if(impl.timeout_opt.idle_timeout == none()) return; - if( impl.timeout_opt.keep_alive_pings && - impl.idle_counter < 1) + if( impl.idle_counter < 1 ) { + if( impl.timeout_opt.keep_alive_pings ) { BOOST_ASIO_HANDLER_LOCATION(( __FILE__, __LINE__, diff --git a/include/boost/beast/websocket/stream_base.hpp b/include/boost/beast/websocket/stream_base.hpp index 0968e80e49..2b7f88ff53 100644 --- a/include/boost/beast/websocket/stream_base.hpp +++ b/include/boost/beast/websocket/stream_base.hpp @@ -116,8 +116,10 @@ struct stream_base An outstanding read operation must be pending, which will complete immediately the error @ref beast::error::timeout. - @li When `keep_alive_pings` is `false`, the connection will be closed. - An outstanding read operation must be pending, which will + @li When `keep_alive_pings` is `false`, the connection will + be closed if there has been no activity. Both websocket + message frames and control frames count as activity. An + outstanding read operation must be pending, which will complete immediately the error @ref beast::error::timeout. */ bool keep_alive_pings; diff --git a/test/beast/websocket/ping.cpp b/test/beast/websocket/ping.cpp index 0bfb5586f4..113fdb9900 100644 --- a/test/beast/websocket/ping.cpp +++ b/test/beast/websocket/ping.cpp @@ -88,6 +88,79 @@ class ping_test : public websocket_test_suite se.code().message()); } } + + // inactivity timeout doesn't happen when you get pings + { + echo_server es{log}; + stream ws{ioc_}; + + // We have an inactivity timeout of 2s, but don't send pings + ws.set_option(stream_base::timeout{ + stream_base::none(), + std::chrono::milliseconds(2000), + false + }); + ws.next_layer().connect(es.stream()); + ws.handshake("localhost", "/"); + flat_buffer b; + bool got_timeout = false; + ws.async_read(b, + [&](error_code ec, std::size_t) + { + if(ec != beast::error::timeout) + BOOST_THROW_EXCEPTION( + system_error{ec}); + got_timeout = true; + }); + // We are connected, base state + 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); + BEAST_EXPECT(!got_timeout); + + test::run_for(ioc_, std::chrono::milliseconds(750)); + // At 3s total; should have triggered the idle timer again without + // activity and triggered timeout. + BEAST_EXPECT(got_timeout); + } + + // inactivity timeout doesn't happen when you send pings + { + echo_server es{log}; + stream ws{ioc_}; + ws.set_option(stream_base::timeout{ + stream_base::none(), + std::chrono::milliseconds(600), + true + }); + unsigned n_pongs = 0; + ws.control_callback({[&](frame_type kind, string_view) + { + if (kind == frame_type::pong) + ++n_pongs; + }}); + ws.next_layer().connect(es.stream()); + ws.handshake("localhost", "/"); + flat_buffer b; + ws.async_read(b, test::fail_handler(asio::error::operation_aborted)); + // We are connected, base state + test::run_for(ioc_, std::chrono::seconds(1)); + // About a second later, we should have close to 5 pings/pongs, and no timeout + BEAST_EXPECTS(2 <= n_pongs && n_pongs <= 3, "Unexpected nr of pings: " + std::to_string(n_pongs)); + } } void diff --git a/test/beast/websocket/test.hpp b/test/beast/websocket/test.hpp index 1575db32bb..3ea25ec23a 100644 --- a/test/beast/websocket/test.hpp +++ b/test/beast/websocket/test.hpp @@ -134,6 +134,12 @@ class websocket_test_suite this)); } + void + async_ping() + { + ws_.async_ping("", [](error_code){}); + } + void async_close() {