From 772797155b7084cf7685a0b322ba8cfed8a83c05 Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Sat, 23 Aug 2025 22:39:14 -0400 Subject: [PATCH] Add on_write_buffer_drained --- src/connection_handler.hpp | 37 ++++++++++++++++++++++++++++++++----- src/http_handler.cpp | 11 +++++------ src/http_handler.hpp | 2 +- src/server.cpp | 12 +++++++++++- style.md | 15 +++++++++++++++ tests/test_http_handler.cpp | 32 ++++++++++++++++++++++++++++++++ 6 files changed, 96 insertions(+), 13 deletions(-) diff --git a/src/connection_handler.hpp b/src/connection_handler.hpp index 4799e60..6dcb347 100644 --- a/src/connection_handler.hpp +++ b/src/connection_handler.hpp @@ -43,11 +43,37 @@ public: std::unique_ptr &) {}; /** - * Successfully wrote data on the connection. + * Called when data has been successfully written to the connection. + * + * This is called during write operations to indicate progress, useful for: + * - Streaming large responses (files, data feeds, etc.) + * - Implementing backpressure for continuous data streams + * - Progress monitoring for long-running transfers + * + * @param conn_ptr Connection that made write progress - handler can take + * ownership * @note May be called from an arbitrary server thread. + * @note Called during writes, not necessarily when buffer becomes empty */ virtual void on_write_progress(std::unique_ptr &) {} + /** + * Called when the connection's outgoing write buffer becomes empty. + * + * This indicates all queued messages have been successfully written + * to the socket. Useful for: + * - Resetting arena allocators safely + * - Implementing keep-alive connection reuse + * - Closing connections after final response + * - Relieving backpressure conditions + * + * @param conn_ptr Connection with empty write buffer - handler can take + * ownership + * @note May be called from an arbitrary server thread. + * @note Only called on transitions from non-empty → empty buffer + */ + virtual void on_write_buffer_drained(std::unique_ptr &) {} + /** * Called when a new connection is established. * @@ -73,10 +99,11 @@ public: /** * @brief Called after a batch of connections has been processed. * - * This hook is called after on_data_arrived or on_write_progress has been - * called for each connection in the batch. The handler can take ownership of - * the connections by moving the unique_ptr out of the span. Any connections - * left in the span will remain owned by the server. + * This hook is called after on_data_arrived, on_write_progress, or + * on_write_buffer_drained has been called for each connection in the batch. + * The handler can take ownership of the connections by moving the unique_ptr + * out of the span. Any connections left in the span will remain owned by the + * server. * * @param batch A span of unique_ptrs to the connections in the batch. */ diff --git a/src/http_handler.cpp b/src/http_handler.cpp index cb6d811..7e758bf 100644 --- a/src/http_handler.cpp +++ b/src/http_handler.cpp @@ -42,13 +42,12 @@ void HttpHandler::on_connection_closed(Connection &conn) { conn.user_data = nullptr; } -void HttpHandler::on_write_progress(std::unique_ptr &conn_ptr) { +void HttpHandler::on_write_buffer_drained( + std::unique_ptr &conn_ptr) { // Reset arena after all messages have been written for the next request - if (conn_ptr->outgoingBytesQueued() == 0) { - on_connection_closed(*conn_ptr); - conn_ptr->reset(); - on_connection_established(*conn_ptr); - } + on_connection_closed(*conn_ptr); + conn_ptr->reset(); + on_connection_established(*conn_ptr); } void HttpHandler::on_post_batch(std::span> batch) { diff --git a/src/http_handler.hpp b/src/http_handler.hpp index 3651201..397a899 100644 --- a/src/http_handler.hpp +++ b/src/http_handler.hpp @@ -102,7 +102,7 @@ struct HttpHandler : ConnectionHandler { void on_connection_closed(Connection &conn) override; void on_data_arrived(std::string_view data, std::unique_ptr &conn_ptr) override; - void on_write_progress(std::unique_ptr &conn_ptr) override; + void on_write_buffer_drained(std::unique_ptr &conn_ptr) override; void on_post_batch(std::span> /*batch*/) override; // Route parsing (public for testing) diff --git a/src/server.cpp b/src/server.cpp index ad19757..3081028 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -485,6 +485,7 @@ void Server::process_connection_writes(std::unique_ptr &conn, // Send immediately if we have outgoing messages (either from EPOLLOUT or // after reading) if ((events & EPOLLOUT) || ((events & EPOLLIN) && conn->hasMessages())) { + bool had_messages = conn->hasMessages(); bool error = conn->writeBytes(); if (error) { conn.reset(); // Connection should be closed @@ -499,8 +500,17 @@ void Server::process_connection_writes(std::unique_ptr &conn, return; } + // Check if buffer became empty (transition from non-empty -> empty) + if (had_messages && !conn->hasMessages()) { + handler_.on_write_buffer_drained(conn); + // If handler took ownership (conn is now null), return + if (!conn) { + return; + } + } + // Check if we should close the connection according to application - if (!conn->hasMessages() && conn->closeConnection_) { + if (!conn->hasMessages() && conn->shouldClose()) { conn.reset(); // Connection should be closed return; } diff --git a/style.md b/style.md index b1bc85d..2034b35 100644 --- a/style.md +++ b/style.md @@ -522,6 +522,21 @@ TEST_CASE("Server accepts connections") { // TEST_CASE("Server creates epoll instance") { /* implementation detail */ } ``` +### What NOT to Test + +**Avoid testing language features and plumbing:** +- Don't test that virtual functions dispatch correctly +- Don't test that standard library types work (unique_ptr, containers, etc.) +- Don't test basic constructor/destructor calls + +**Test business logic instead:** +- When does your code call hooks/callbacks and why? +- What state transitions trigger behavior changes? +- How does your code handle error conditions? +- What promises does your API make to users? + +**Ask: "Am I testing the C++ compiler or my application logic?"** + ### Test Synchronization (Authoritative Rules) - **ABSOLUTELY NEVER use timeouts** (`sleep_for`, `wait_for`, etc.) - **Deterministic synchronization only:** diff --git a/tests/test_http_handler.cpp b/tests/test_http_handler.cpp index 84cc5c0..d21b603 100644 --- a/tests/test_http_handler.cpp +++ b/tests/test_http_handler.cpp @@ -88,3 +88,35 @@ TEST_CASE("HttpHandler route parsing edge cases") { HttpRoute::GET_retention); } } + +// Test helper to verify the new hook functionality +struct MockConnectionHandler : public ConnectionHandler { + bool write_progress_called = false; + bool write_buffer_drained_called = false; + + void on_write_progress(std::unique_ptr &) override { + write_progress_called = true; + } + + void on_write_buffer_drained(std::unique_ptr &) override { + write_buffer_drained_called = true; + } +}; + +TEST_CASE("ConnectionHandler hooks") { + SUBCASE("on_write_buffer_drained hook exists") { + MockConnectionHandler handler; + + // Verify hooks are available and can be overridden + CHECK_FALSE(handler.write_progress_called); + CHECK_FALSE(handler.write_buffer_drained_called); + + // Would normally be called by Server during write operations + std::unique_ptr null_conn; + handler.on_write_progress(null_conn); + handler.on_write_buffer_drained(null_conn); + + CHECK(handler.write_progress_called); + CHECK(handler.write_buffer_drained_called); + } +}