From: Thomas Markwalder Date: Fri, 4 Nov 2022 19:19:28 +0000 (-0400) Subject: [#2583] Removed request timeout X-Git-Tag: Kea-2.3.3~99 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=64fa4a1251b0ba6735002b2147c19382c33b97cf;p=thirdparty%2Fkea.git [#2583] Removed request timeout src/lib/tcp/tcp_connection.* src/lib/tcp/tcp_listener.* src/lib/tcp/tests/tcp_listener_unittests.cc Removed request timeout and test --- diff --git a/src/lib/tcp/tcp_connection.cc b/src/lib/tcp/tcp_connection.cc index 0b5edd5e57..18a00f6461 100644 --- a/src/lib/tcp/tcp_connection.cc +++ b/src/lib/tcp/tcp_connection.cc @@ -49,17 +49,15 @@ SocketCallback::operator()(boost::system::error_code ec, size_t length) { } TcpConnection::TcpConnection(asiolink::IOService& io_service, - const TcpConnectionAcceptorPtr& acceptor, - const TlsContextPtr& tls_context, - TcpConnectionPool& connection_pool, - const TcpConnectionAcceptorCallback& callback, - const long request_timeout, - const long idle_timeout, - const size_t read_max /* = 32768 */) - : request_timer_(io_service), - request_timeout_(request_timeout), - tls_context_(tls_context), + const TcpConnectionAcceptorPtr& acceptor, + const TlsContextPtr& tls_context, + TcpConnectionPool& connection_pool, + const TcpConnectionAcceptorCallback& callback, + const long idle_timeout, + const size_t read_max /* = 32768 */) + : tls_context_(tls_context), idle_timeout_(idle_timeout), + idle_timer_(io_service), tcp_socket_(), tls_socket_(), acceptor_(acceptor), @@ -87,7 +85,7 @@ TcpConnection::shutdownCallback(const boost::system::error_code&) { void TcpConnection::shutdown() { - request_timer_.cancel(); + idle_timer_.cancel(); if (tcp_socket_) { tcp_socket_->close(); return; @@ -108,7 +106,7 @@ TcpConnection::shutdown() { void TcpConnection::close() { - request_timer_.cancel(); + idle_timer_.cancel(); if (tcp_socket_) { tcp_socket_->close(); return; @@ -180,11 +178,13 @@ TcpConnection::doHandshake() { // Skip the handshake if the socket is not a TLS one. if (!tls_socket_) { HERE(""); - setupIdleTimer(); doRead(); return; } + HERE("setupIdleTimer"); + setupIdleTimer(); + // Create instance of the callback. It is safe to pass the local instance // of the callback, because the underlying boost functions make copies // as needed. @@ -206,14 +206,13 @@ TcpConnection::doRead(TcpRequestPtr request) { HERE(""); TCPEndpoint endpoint; + HERE("setup Idle timer"); + setupIdleTimer(); + // Request hasn't been created if we are starting to read the // new request. if (!request) { - HERE("new read start - setup Idle timer"); - setupIdleTimer(); request = createRequest(); - } else { - setupRequestTimer(); } // Create instance of the callback. It is safe to pass the local instance @@ -268,8 +267,7 @@ TcpConnection::doWrite(TcpResponsePtr response) { return; } } else { - // The connection remains open and we are done sending - // the response. Start listening for the next requests. + // The connection remains open and we are done sending the response. HERE("setupIdleTimer - write finsihed"); setupIdleTimer(); } @@ -302,16 +300,14 @@ TcpConnection::acceptorCallback(const boost::system::error_code& ec) { LOG_DEBUG(tcp_logger, isc::log::DBGLVL_TRACE_DETAIL, TCP_REQUEST_RECEIVE_START) .arg(getRemoteEndpointAddressAsText()) - .arg(static_cast(request_timeout_/1000)); + .arg(static_cast(idle_timeout_/1000)); } else { LOG_DEBUG(tcp_logger, isc::log::DBGLVL_TRACE_DETAIL, TCP_CONNECTION_HANDSHAKE_START) .arg(getRemoteEndpointAddressAsText()) - .arg(static_cast(request_timeout_/1000)); + .arg(static_cast(idle_timeout_/1000)); } - HERE("setupIdleTimer"); - setupIdleTimer(); doHandshake(); } } @@ -358,8 +354,8 @@ TcpConnection::socketReadCallback(TcpRequestPtr request, } // Data received, Restart the request timer. - HERE("setupRequestTimer - data recevied, post it") - setupRequestTimer(request); + HERE("setupIdleTimer - data recevied, post it") + setupIdleTimer(); TcpRequestPtr next_request = request; if (length) { @@ -387,9 +383,8 @@ TcpConnection::postData(TcpRequestPtr request, WireData& input_data) { } if (request->needData()) { - // Current request is incomplete and we're out of data. Restart the timer - // and return the incomplete request. - setupRequestTimer(); + // Current request is incomplete and we're out of data + // return the incomplete request and we'll read again. return (request); } @@ -404,8 +399,8 @@ TcpConnection::postData(TcpRequestPtr request, WireData& input_data) { .arg(request->logFormatRequest(MAX_LOGGED_MESSAGE_SIZE)); // Request complete, stop the timer. - HERE("Cancel requestTimer to handle complete request"); - request_timer_.cancel(); + HERE("Cancel idleTimer while we handle complete request"); + idle_timer_.cancel(); // Process the completed request. requestReceived(request); @@ -426,13 +421,6 @@ TcpConnection::postData(TcpRequestPtr request, WireData& input_data) { // The input buffer spanned messages. Recurse to post the remainder to the // new request. request = postData(request, input_data); - // Restart the request timer. - HERE("spanning read, start request timer"); - setupRequestTimer(); - } else { - // No incomplete requests, start the idle timer. - HERE("no waiting requests, start idle timer"); - setupIdleTimer(); } return (request); @@ -477,35 +465,11 @@ TcpConnection::socketWriteCallback(TcpResponsePtr response, doWrite(response); } -void -TcpConnection::setupRequestTimer(TcpRequestPtr request) { - // Pass raw pointer rather than shared_ptr to this object, - // because IntervalTimer already passes shared pointer to the - // IntervalTimerImpl to make sure that the callback remains - // valid. - request_timer_.setup(std::bind(&TcpConnection::requestTimeoutCallback, this, request), - request_timeout_, IntervalTimer::ONE_SHOT); -} - void TcpConnection::setupIdleTimer() { - request_timer_.setup(std::bind(&TcpConnection::idleTimeoutCallback, this), - idle_timeout_, IntervalTimer::ONE_SHOT); -} - -void -TcpConnection::requestTimeoutCallback(TcpRequestPtr /* request */) { - LOG_DEBUG(tcp_logger, isc::log::DBGLVL_TRACE_DETAIL, - TCP_CLIENT_REQUEST_TIMEOUT_OCCURRED) - .arg(getRemoteEndpointAddressAsText()); - /// @todo Not sure what is meaningful to do here. If client - /// comes back and finishes sending we'll use the first bytes - /// as length, but they'll be meaningless. How would one - /// get back on track? Seems like the safest thing would be - /// to disconnect them, like idle timeout. - /// need a handler here that can be overridden - maybe BLQ should - /// send back a mal-formed error? - HERE("Request timeout! Discarding partial request"); + std::cout << "idle timeout: " << idle_timeout_ << std::endl; + idle_timer_.setup(std::bind(&TcpConnection::idleTimeoutCallback, this), + idle_timeout_, IntervalTimer::ONE_SHOT); } void diff --git a/src/lib/tcp/tcp_connection.h b/src/lib/tcp/tcp_connection.h index 38709b1f73..2bfbea038e 100644 --- a/src/lib/tcp/tcp_connection.h +++ b/src/lib/tcp/tcp_connection.h @@ -212,18 +212,16 @@ public: /// @param connection_pool Connection pool in which this connection is /// stored. /// @param callback Callback invoked when new connection is accepted. - /// @param request_timeout Configured timeout for a TCP request. /// @param idle_timeout Timeout after which a TCP connection is /// closed by the server. /// @param read_max maximum size of a single socket read. Defaults to 32K. TcpConnection(asiolink::IOService& io_service, - const TcpConnectionAcceptorPtr& acceptor, - const asiolink::TlsContextPtr& tls_context, - TcpConnectionPool& connection_pool, - const TcpConnectionAcceptorCallback& callback, - const long request_timeout, - const long idle_timeout, - const size_t read_max = 32768); + const TcpConnectionAcceptorPtr& acceptor, + const asiolink::TlsContextPtr& tls_context, + TcpConnectionPool& connection_pool, + const TcpConnectionAcceptorCallback& callback, + const long idle_timeout, + const size_t read_max = 32768); /// @brief Destructor. /// @@ -375,22 +373,9 @@ protected: /// @param ec Error code (ignored). void shutdownCallback(const boost::system::error_code& ec); - /// @brief Reset timer for detecting request timeouts. - // - /// @param request Pointer to the request to be guarded by the timeout. - void setupRequestTimer(TcpRequestPtr request = TcpRequestPtr()); - /// @brief Reset timer for detecting idle timeout in connections. void setupIdleTimer(); - /// @brief Callback invoked when the TCP Request Timeout occurs. - /// - /// This callback creates TCP response with Request Timeout error code - /// and sends it to the client. - /// - /// @param request Pointer to the request for which timeout occurs. - void requestTimeoutCallback(TcpRequestPtr request); - /// @brief Callback invoked when the client has been idle. void idleTimeoutCallback(); @@ -421,12 +406,6 @@ protected: return (input_buf_.size()); } - /// @brief Timer used to detect Request Timeout. - asiolink::IntervalTimer request_timer_; - - /// @brief Configured Request Timeout in milliseconds. - long request_timeout_; - /// @brief TLS context. asiolink::TlsContextPtr tls_context_; @@ -434,6 +413,9 @@ protected: /// down by the server. long idle_timeout_; + /// @brief Timer used to detect idle Timeout. + asiolink::IntervalTimer idle_timer_; + /// @brief TCP socket used by this connection. std::unique_ptr > tcp_socket_; diff --git a/src/lib/tcp/tcp_listener.cc b/src/lib/tcp/tcp_listener.cc index 5f13892551..4a6543dd27 100644 --- a/src/lib/tcp/tcp_listener.cc +++ b/src/lib/tcp/tcp_listener.cc @@ -18,11 +18,9 @@ TcpListener::TcpListener(IOService& io_service, const IOAddress& server_address, const unsigned short server_port, const TlsContextPtr& tls_context, - const RequestTimeout& request_timeout, const IdleTimeout& idle_timeout) : io_service_(io_service), tls_context_(tls_context), acceptor_(), - endpoint_(), connections_(), request_timeout_(request_timeout.value_), - idle_timeout_(idle_timeout.value_) { + endpoint_(), connections_(), idle_timeout_(idle_timeout.value_) { // Create the TCP or TLS acceptor. if (!tls_context) { @@ -39,12 +37,6 @@ TcpListener::TcpListener(IOService& io_service, << server_address << ":" << server_port); } - // Request timeout is signed and must be greater than 0. - if (request_timeout_ <= 0) { - isc_throw(TcpListenerError, "Invalid desired TCP request timeout " - << request_timeout_); - } - // Idle connection timeout is signed and must be greater than 0. if (idle_timeout_ <= 0) { isc_throw(TcpListenerError, "Invalid desired TCP idle connection" diff --git a/src/lib/tcp/tcp_listener.h b/src/lib/tcp/tcp_listener.h index b13f1a019b..dfc3b35816 100644 --- a/src/lib/tcp/tcp_listener.h +++ b/src/lib/tcp/tcp_listener.h @@ -28,17 +28,6 @@ public: /// that each connection does its client's work on it's own thread. class TcpListener { public: - /// @brief TCP request timeout value. - struct RequestTimeout { - /// @brief Constructor. - /// - /// @param value Request timeout value in milliseconds. - explicit RequestTimeout(long value) - : value_(value) { - } - long value_; ///< Request timeout value specified. - }; - /// @brief Idle connection timeout. struct IdleTimeout { /// @brief Constructor. @@ -62,8 +51,6 @@ public: /// @param server_address Address on which the TCP service should run. /// @param server_port Port number on which the TCP service should run. /// @param tls_context TLS context. - /// @param request_timeout Timeout maximum amount of time allotted for - /// a request to be processed. /// @param idle_timeout Timeout after which an idle TCP connection is /// closed by the server. /// @@ -73,7 +60,6 @@ public: const asiolink::IOAddress& server_address, const unsigned short server_port, const asiolink::TlsContextPtr& tls_context, - const RequestTimeout& request_timeout, const IdleTimeout& idle_timeout); /// @brief Virtual destructor. @@ -143,9 +129,6 @@ protected: /// @brief Pool of active connections. TcpConnectionPool connections_; - /// @brief Maximum amount of time request to be processed. - long request_timeout_; - /// @brief Timeout after which idle connection is closed by /// the server. long idle_timeout_; diff --git a/src/lib/tcp/tcp_messages.cc b/src/lib/tcp/tcp_messages.cc index b66fd55bc8..6b9f2bcc26 100644 --- a/src/lib/tcp/tcp_messages.cc +++ b/src/lib/tcp/tcp_messages.cc @@ -11,7 +11,6 @@ extern const isc::log::MessageID TCP_BAD_CLIENT_REQUEST_RECEIVED = "TCP_BAD_CLIE extern const isc::log::MessageID TCP_BAD_CLIENT_REQUEST_RECEIVED_DETAILS = "TCP_BAD_CLIENT_REQUEST_RECEIVED_DETAILS"; extern const isc::log::MessageID TCP_CLIENT_REQUEST_RECEIVED = "TCP_CLIENT_REQUEST_RECEIVED"; extern const isc::log::MessageID TCP_CLIENT_REQUEST_RECEIVED_DETAILS = "TCP_CLIENT_REQUEST_RECEIVED_DETAILS"; -extern const isc::log::MessageID TCP_CLIENT_REQUEST_TIMEOUT_OCCURRED = "TCP_CLIENT_REQUEST_TIMEOUT_OCCURRED"; extern const isc::log::MessageID TCP_CONNECTION_CLOSE_CALLBACK_FAILED = "TCP_CONNECTION_CLOSE_CALLBACK_FAILED"; extern const isc::log::MessageID TCP_CONNECTION_HANDSHAKE_FAILED = "TCP_CONNECTION_HANDSHAKE_FAILED"; extern const isc::log::MessageID TCP_CONNECTION_HANDSHAKE_START = "TCP_CONNECTION_HANDSHAKE_START"; @@ -36,7 +35,6 @@ const char* values[] = { "TCP_BAD_CLIENT_REQUEST_RECEIVED_DETAILS", "detailed information about malformed request received from %1:\n%2", "TCP_CLIENT_REQUEST_RECEIVED", "received TCP request from %1", "TCP_CLIENT_REQUEST_RECEIVED_DETAILS", "detailed information about well-formed request received from %1:\n%2", - "TCP_CLIENT_REQUEST_TIMEOUT_OCCURRED", "Timeout occurred while receiving a client request", "TCP_CONNECTION_CLOSE_CALLBACK_FAILED", "Connection close callback threw an exception", "TCP_CONNECTION_HANDSHAKE_FAILED", "TLS handshake with %1 failed with %2", "TCP_CONNECTION_HANDSHAKE_START", "start TLS handshake with %1 with timeout %2", diff --git a/src/lib/tcp/tcp_messages.h b/src/lib/tcp/tcp_messages.h index 00c6f2fddc..16ea952bd6 100644 --- a/src/lib/tcp/tcp_messages.h +++ b/src/lib/tcp/tcp_messages.h @@ -12,7 +12,6 @@ extern const isc::log::MessageID TCP_BAD_CLIENT_REQUEST_RECEIVED; extern const isc::log::MessageID TCP_BAD_CLIENT_REQUEST_RECEIVED_DETAILS; extern const isc::log::MessageID TCP_CLIENT_REQUEST_RECEIVED; extern const isc::log::MessageID TCP_CLIENT_REQUEST_RECEIVED_DETAILS; -extern const isc::log::MessageID TCP_CLIENT_REQUEST_TIMEOUT_OCCURRED; extern const isc::log::MessageID TCP_CONNECTION_CLOSE_CALLBACK_FAILED; extern const isc::log::MessageID TCP_CONNECTION_HANDSHAKE_FAILED; extern const isc::log::MessageID TCP_CONNECTION_HANDSHAKE_START; diff --git a/src/lib/tcp/tcp_messages.mes b/src/lib/tcp/tcp_messages.mes index 3447b80a89..5c5cda9678 100644 --- a/src/lib/tcp/tcp_messages.mes +++ b/src/lib/tcp/tcp_messages.mes @@ -72,9 +72,6 @@ of the request. The first argument specifies the amount of received data. The second argument specifies an address of the remote endpoint which produced the data. -% TCP_CLIENT_REQUEST_TIMEOUT_OCCURRED Timeout occurred while receiving a client request -This debug message is issued when a timeout occurs during the reception of request - % TCP_IDLE_CONNECTION_TIMEOUT_OCCURRED closing connection with %1 as a result of a timeout This debug message is issued when the TCP connection is being closed as a result of being idle. diff --git a/src/lib/tcp/tests/tcp_listener_unittests.cc b/src/lib/tcp/tests/tcp_listener_unittests.cc index 54ef3f4794..8bbdc5d4cb 100644 --- a/src/lib/tcp/tests/tcp_listener_unittests.cc +++ b/src/lib/tcp/tests/tcp_listener_unittests.cc @@ -70,10 +70,9 @@ public: const TlsContextPtr& tls_context, TcpConnectionPool& connection_pool, const TcpConnectionAcceptorCallback& callback, - const long request_timeout, const long idle_timeout) : TcpConnection(io_service, acceptor, tls_context, connection_pool, callback, - request_timeout, idle_timeout) { + idle_timeout) { } virtual TcpRequestPtr createRequest() { @@ -114,11 +113,10 @@ public: const IOAddress& server_address, const unsigned short server_port, const TlsContextPtr& tls_context, - const RequestTimeout& request_timeout, const IdleTimeout& idle_timeout, const size_t read_max = 32 * 1024) : TcpListener(io_service, server_address, server_port, - tls_context, request_timeout, idle_timeout), + tls_context, idle_timeout), read_max_(read_max) { } @@ -135,7 +133,7 @@ protected: TcpConnectionPtr conn(new TcpTestConnection(io_service_, acceptor_, tls_context_, connections_, - callback, request_timeout_, idle_timeout_)); + callback, idle_timeout_)); conn->setReadMax(read_max_); return (conn); } @@ -241,8 +239,7 @@ TEST_F(TcpListenerTest, listen) { const std::string request = "I am done"; TcpTestListener listener(io_service_, IOAddress(SERVER_ADDRESS), SERVER_PORT, - TlsContextPtr(), TcpListener::RequestTimeout(REQUEST_TIMEOUT), - TcpListener::IdleTimeout(IDLE_TIMEOUT)); + TlsContextPtr(), TcpListener::IdleTimeout(IDLE_TIMEOUT)); ASSERT_NO_THROW(listener.start()); ASSERT_EQ(SERVER_ADDRESS, listener.getLocalAddress().toText()); @@ -267,8 +264,7 @@ TEST_F(TcpListenerTest, splitReads) { // Read at most one byte at a time. size_t read_max = 1; TcpTestListener listener(io_service_, IOAddress(SERVER_ADDRESS), SERVER_PORT, - TlsContextPtr(), TcpListener::RequestTimeout(REQUEST_TIMEOUT), - TcpListener::IdleTimeout(IDLE_TIMEOUT), read_max); + TlsContextPtr(), TcpListener::IdleTimeout(IDLE_TIMEOUT), read_max); ASSERT_NO_THROW(listener.start()); ASSERT_EQ(SERVER_ADDRESS, listener.getLocalAddress().toText()); @@ -291,8 +287,7 @@ TEST_F(TcpListenerTest, splitReads) { // transmit a streamed request and receive a streamed response. TEST_F(TcpListenerTest, idleTimeoutTest) { TcpTestListener listener(io_service_, IOAddress(SERVER_ADDRESS), SERVER_PORT, - TlsContextPtr(), TcpListener::RequestTimeout(REQUEST_TIMEOUT), - TcpListener::IdleTimeout(SHORT_IDLE_TIMEOUT)); + TlsContextPtr(), TcpListener::IdleTimeout(SHORT_IDLE_TIMEOUT)); ASSERT_NO_THROW(listener.start()); ASSERT_EQ(SERVER_ADDRESS, listener.getLocalAddress().toText()); @@ -314,35 +309,4 @@ TEST_F(TcpListenerTest, idleTimeoutTest) { io_service_.poll(); } -// @todo TKM - Disabled until request timeout handler mechanism is established. -// In it's current form the request timeout occurs correctly but without a server -// reaction (such as close or error response) the test timeout expires. -// This test verifies that A TCP request can time out if not received -// completely. -TEST_F(TcpListenerTest, DISABLED_requestTimeoutTest) { - TcpTestListener listener(io_service_, IOAddress(SERVER_ADDRESS), SERVER_PORT, - TlsContextPtr(), TcpListener::RequestTimeout(SHORT_REQUEST_TIMEOUT), - TcpListener::IdleTimeout(IDLE_TIMEOUT)); - - ASSERT_NO_THROW(listener.start()); - ASSERT_EQ(SERVER_ADDRESS, listener.getLocalAddress().toText()); - ASSERT_EQ(SERVER_PORT, listener.getLocalPort()); - ASSERT_NO_THROW(connectClient()); - ASSERT_EQ(1, clients_.size()); - TcpTestClientPtr client = *clients_.begin(); - ASSERT_TRUE(client); - - // Send part of a request. - const std::string request = "I am done"; - ASSERT_NO_THROW(client->sendRequest(request, (request.size()/2))); - - // Run until idle timer expires. - ASSERT_NO_THROW(runIOService()); - EXPECT_FALSE(client->receiveDone()); - EXPECT_FALSE(client->expectedEof()); - - listener.stop(); - io_service_.poll(); -} - }