From b7282a2f03cd59e21bd5b3f9f52c96ad531d99ca Mon Sep 17 00:00:00 2001 From: Andrew Noyes Date: Tue, 19 Aug 2025 17:20:36 -0400 Subject: [PATCH] Add thread safety documentation --- design.md | 20 ++- src/arena_allocator.cpp | 1 + src/arena_allocator.hpp | 34 ++++- src/connection.cpp | 10 ++ src/connection.hpp | 289 +++++++++++++++++++++++++++++++++++++++- src/server.cpp | 2 +- 6 files changed, 347 insertions(+), 9 deletions(-) diff --git a/design.md b/design.md index 41e8013..f33ccc5 100644 --- a/design.md +++ b/design.md @@ -56,6 +56,7 @@ Key features: - Proper alignment handling for all types - Move semantics for efficient transfers - Requires trivially destructible types only +- Thread-safe per-connection usage via exclusive ownership model #### 2. **Commit Request Data Model** (`src/commit_request.hpp`) - **Format-agnostic data structure** for representing transactional commits @@ -265,6 +266,7 @@ The modular design allows each component to be optimized independently while mai ### Important Implementation Details - **Server Creation**: Always use `Server::create()` factory method - direct construction is impossible +- **Connection Creation**: Only the Server can create connections - no public constructor or factory method - **Connection Ownership**: Use unique_ptr semantics for safe ownership transfer between components - **Arena Allocator Pattern**: Always use `ArenaAllocator` for temporary allocations within request processing - **String View Usage**: Prefer `std::string_view` over `std::string` when pointing to arena-allocated memory @@ -323,7 +325,9 @@ The modular design allows each component to be optimized independently while mai ## Common Patterns -### Server Creation Pattern +### Factory Method Patterns + +#### Server Creation ```cpp // Server must be created via factory method auto server = Server::create(config, handler); @@ -333,6 +337,20 @@ auto server = Server::create(config, handler); // auto server = std::make_shared(config, handler); // Compiler error ``` +#### Connection Creation (Server-Only) +```cpp +// Only Server can create connections (using private friend method) +class Server { +private: + auto conn = Connection::createForServer(addr, fd, id, handler, weak_from_this()); +}; + +// No public way to create connections - all these fail: +// auto conn = Connection::create(...); // ERROR: no such method +// Connection conn(addr, fd, id, handler, server); // ERROR: private constructor +// auto conn = std::make_unique(...); // ERROR: private constructor +``` + ### ConnectionHandler Implementation Patterns #### Simple Synchronous Handler diff --git a/src/arena_allocator.cpp b/src/arena_allocator.cpp index 9de4a2c..ddac348 100644 --- a/src/arena_allocator.cpp +++ b/src/arena_allocator.cpp @@ -2,6 +2,7 @@ #include #include #include +#include ArenaAllocator::~ArenaAllocator() { while (current_block_) { diff --git a/src/arena_allocator.hpp b/src/arena_allocator.hpp index a51be67..d5828d1 100644 --- a/src/arena_allocator.hpp +++ b/src/arena_allocator.hpp @@ -12,7 +12,6 @@ #include #include #include -#include /** * @brief A high-performance arena allocator for bulk allocations. @@ -52,8 +51,37 @@ * - Move semantics transfer ownership of all blocks * * ## Thread Safety: - * Not thread-safe. Use separate instances per thread or external - * synchronization. + * ArenaAllocator is **not thread-safe** - concurrent access from multiple + * threads requires external synchronization. However, this design is + * intentional for performance reasons and the WeaselDB architecture ensures + * thread safety through ownership patterns: + * + * ### Safe Usage Patterns in WeaselDB: + * - **Per-Connection Instances**: Each Connection owns its own ArenaAllocator + * instance, accessed only by the thread that currently owns the connection + * - **Single Owner Principle**: Connection ownership transfers atomically + * between threads using unique_ptr, ensuring only one thread accesses the arena + * at a time + * + * ### Thread Ownership Model: + * 1. **Network Thread**: Claims connection ownership, accesses arena for I/O + * buffers + * 2. **Handler Thread**: Can take ownership via unique_ptr.release(), uses + * arena for request parsing and response generation + * 3. **Background Thread**: Can receive ownership for async processing, uses + * arena for temporary data structures + * 4. **Return Path**: Connection (and its arena) safely returned via + * Server::releaseBackToServer() + * + * ### Why This Design is Thread-Safe: + * - **Exclusive Access**: Only the current owner thread should access the arena + * - **Transfer Points**: Ownership transfers happen at well-defined + * synchronization points with proper memory barriers. + * - **No Shared State**: Each arena is completely isolated - no shared data + * between different arena instances + * + * @warning Do not share ArenaAllocator instances between threads. Use separate + * instances per thread or per logical unit of work. */ class ArenaAllocator { private: diff --git a/src/connection.cpp b/src/connection.cpp index 19a5f58..78de4d4 100644 --- a/src/connection.cpp +++ b/src/connection.cpp @@ -5,6 +5,16 @@ #include #include +std::unique_ptr +Connection::createForServer(struct sockaddr_storage addr, int fd, int64_t id, + ConnectionHandler *handler, + std::weak_ptr server) { + // Use unique_ptr constructor with private access (friend function) + // We can't use make_unique here because constructor is private + return std::unique_ptr( + new Connection(addr, fd, id, handler, server)); +} + Connection::Connection(struct sockaddr_storage addr, int fd, int64_t id, ConnectionHandler *handler, std::weak_ptr server) : fd_(fd), id_(id), addr_(addr), arena_(), handler_(handler), diff --git a/src/connection.hpp b/src/connection.hpp index 9b693b3..9daa031 100644 --- a/src/connection.hpp +++ b/src/connection.hpp @@ -28,6 +28,15 @@ extern std::atomic activeConnections; * pointer * - RAII cleanup happens if network thread doesn't transfer back * + * Arena allocator thread safety: + * Each Connection contains its own ArenaAllocator instance that is accessed + * exclusively by the thread that currently owns the connection. This ensures + * thread safety without requiring locks: + * - Arena is used by the owning thread for I/O buffers, request parsing, and + * response generation + * - Arena memory is automatically freed when the connection is destroyed + * - reset() should only be called by the current owner thread + * * Only the handler interface methods are public - all networking details are * private. */ @@ -35,15 +44,172 @@ extern std::atomic activeConnections; class Server; struct Connection { - Connection(struct sockaddr_storage addr, int fd, int64_t id, - ConnectionHandler *handler, std::weak_ptr server); + // No public constructor or factory method - only Server can create + // connections + + /** + * @brief Destroy the Connection object. + * + * Automatically handles cleanup of all connection resources: + * - Calls handler's on_connection_closed() method for protocol-specific + * cleanup + * - Decrements the global active connection counter + * - Closes the socket file descriptor + * - Frees all arena-allocated memory + * + * @note The destructor will abort() if socket close fails, as this indicates + * a serious system-level issue that should not be ignored. + */ ~Connection(); // Handler interface - public methods that handlers can use + + /** + * @brief Queue a message to be sent to the client. + * + * Adds data to the connection's outgoing message queue. The data will be sent + * asynchronously by the server's network threads using efficient vectored + * I/O. + * + * @param s The data to send (string view for zero-copy efficiency) + * @param copyToArena If true (default), copies data to the connection's arena + * for safe storage. If false, the caller must ensure the + * data remains valid until all queued messages are sent. + * + * @warning Thread Safety: Only call from the thread that currently owns this + * connection. The arena allocator is not thread-safe. + * + * @note Performance: Use copyToArena=false for static strings or data with + * guaranteed lifetime, copyToArena=true for temporary/dynamic data. + * + * Example usage: + * ```cpp + * conn->appendMessage("HTTP/1.1 200 OK\r\n\r\n", false); // Static string + * conn->appendMessage(dynamic_response, true); // Dynamic data + * conn->appendMessage(arena_allocated_data, false); // Arena data + * ``` + */ void appendMessage(std::string_view s, bool copyToArena = true); + + /** + * @brief Mark the connection to be closed after sending all queued messages. + * + * Sets a flag that instructs the server to close this connection gracefully + * after all currently queued messages have been successfully sent to the + * client. This enables proper connection cleanup for protocols like HTTP/1.0 + * or when implementing connection limits. + * + * @note The connection will remain active until: + * 1. All queued messages are sent to the client + * 2. The server processes the close flag during the next I/O cycle + * 3. The connection is properly closed and cleaned up + * + * @warning Thread Safety: Only call from the thread that currently owns this + * connection. + * + * Typical usage: + * ```cpp + * conn->appendMessage("HTTP/1.1 200 OK\r\n\r\nBye!"); + * conn->closeAfterSend(); // Close after sending response + * ``` + */ void closeAfterSend() { closeConnection_ = true; } + + /** + * @brief Get access to the connection's arena allocator. + * + * Returns a reference to this connection's private ArenaAllocator instance, + * which should be used for all temporary allocations during request + * processing. The arena provides extremely fast allocation (~1ns) and + * automatic cleanup when the connection is destroyed or reset. + * + * @return Reference to the connection's arena allocator + * + * @warning Thread Safety: Only access from the thread that currently owns + * this connection. The arena allocator is not thread-safe and concurrent + * access will result in undefined behavior. + * + * @note Memory Lifecycle: Arena memory is automatically freed when: + * - The connection is destroyed + * - reset() is called (keeps first block, frees others) + * - The connection is moved (arena ownership transfers) + * + * Best practices: + * ```cpp + * ArenaAllocator& arena = conn->getArena(); + * + * // Allocate temporary parsing buffers + * char* buffer = arena.allocate(1024); + * + * // Construct temporary objects + * auto* request = arena.construct(arena); + * + * // Use arena-backed STL containers + * std::vector> tokens{&arena}; + * ``` + */ ArenaAllocator &getArena() { return arena_; } + + /** + * @brief Get the unique identifier for this connection. + * + * Returns a server-generated unique ID that can be used for logging, + * debugging, or tracking individual connections. The ID is assigned + * atomically when the connection is created and remains constant throughout + * the connection's lifetime. + * + * @return Unique 64-bit connection identifier + * + * @note Thread Safety: This method is thread-safe as it only reads an + * immutable value set during construction. + * + * @note The ID is generated using an atomic counter, ensuring uniqueness + * across all connections within a single server instance. IDs are not + * guaranteed to be sequential due to concurrent connection creation. + * + * Typical usage: + * ```cpp + * std::cout << "Processing request on connection " << conn->getId() << + * std::endl; logger.info("Connection {} sent {} bytes", conn->getId(), + * response.size()); + * ``` + */ int64_t getId() const { return id_; } + + /** + * @brief Get the number of bytes queued for transmission. + * + * Calculates and returns the total number of bytes in all messages currently + * queued for transmission to the client. This includes all data added via + * appendMessage() that has not yet been sent over the network. + * + * @return Total bytes queued for transmission + * + * @warning Thread Safety: Only call from the thread that currently owns this + * connection. Concurrent access to the message queue is not thread-safe. + * + * @note Performance: This method iterates through all queued messages to + * calculate the total, so avoid calling it frequently in hot paths. + * + * @note The count decreases as the server sends data via writeBytes() and + * removes completed messages from the queue. + * + * Use cases: + * ```cpp + * // Check if all data has been sent + * if (conn->outgoingBytesQueued() == 0) { + * conn->reset(); // Safe to reset arena + * } + * + * // Implement backpressure + * if (conn->outgoingBytesQueued() > MAX_BUFFER_SIZE) { + * // Stop adding more data until queue drains + * } + * + * // Logging/monitoring + * metrics.recordQueueDepth(conn->getId(), conn->outgoingBytesQueued()); + * ``` + */ int64_t outgoingBytesQueued() const { int64_t result = 0; for (auto s : messages_) { @@ -51,7 +217,70 @@ struct Connection { } return result; } + + /** + * @brief Protocol-specific data pointer for handler use. + * + * A void pointer that can be used by ConnectionHandler implementations to + * attach protocol-specific state or data structures to individual + * connections. This enables handlers to maintain per-connection state without + * requiring external lookup tables. + * + * @warning Memory Management: The handler is responsible for allocating and + * freeing any data pointed to by user_data. Typically this should be done in + * on_connection_established() and on_connection_closed() respectively. + * + * @warning Thread Safety: Only access from the thread that currently owns + * this connection. The pointer itself and any referenced data must not be + * accessed concurrently. + * + * @note Lifetime: The user_data pointer is set to nullptr in the destructor + * for safety, but the handler must ensure proper cleanup in + * on_connection_closed() before the connection is destroyed. + * + * Example usage: + * ```cpp + * class HttpHandler : public ConnectionHandler { + * void on_connection_established(Connection& conn) override { + * // Allocate HTTP state in connection's arena or heap + * auto* state = conn.getArena().construct(); + * conn.user_data = state; + * } + * + * void on_connection_closed(Connection& conn) override { + * // Cleanup if using heap allocation + * delete static_cast(conn.user_data); + * conn.user_data = nullptr; + * } + * + * void on_data_arrived(std::string_view data, + * std::unique_ptr& conn_ptr) override { + * auto* state = static_cast(conn_ptr->user_data); + * // Use state for protocol processing... + * } + * }; + * ``` + */ void *user_data = nullptr; + + /** + * Reset the connection's arena allocator and message queue for reuse. + * + * This method efficiently reclaims arena memory by keeping the first block + * and freeing all others, then reinitializes the message queue. + * + * @warning Thread Safety: This method should ONLY be called by the thread + * that currently owns this connection. Calling reset() while the connection + * is being transferred between threads or accessed by another thread will + * result in undefined behavior. + * + * @note The assert(messages_.empty()) ensures all outgoing data has been + * sent before resetting. This prevents data loss and indicates the connection + * is in a clean state for reuse. + * + * Typical usage pattern: + * - HTTP handlers call this after completing a request/response cycle + */ void reset() { assert(messages_.empty()); arena_.reset(); @@ -60,13 +289,65 @@ struct Connection { ArenaStlAllocator{&arena_}}; } - // Note: To release connection back to server, use - // Server::releaseBackToServer(std::move(connection_ptr)) + /** + * @note Ownership Transfer: To release a connection back to the server for + * continued processing, use the static method: + * ```cpp + * Server::releaseBackToServer(std::move(connection_ptr)); + * ``` + * + * This is the correct way to return connection ownership when: + * - A handler has taken ownership via unique_ptr.release() + * - Background processing of the connection is complete + * - The connection should resume normal server-managed I/O processing + * + * The method is thread-safe and handles the case where the server may have + * been destroyed while the connection was being processed elsewhere. + */ private: // Server is a friend and can access all networking internals friend class Server; + /** + * @brief Private constructor - only accessible by Server. + * + * Creates a new connection with the specified network address, file + * descriptor, and associated handler. Automatically increments the global + * active connection counter and calls the handler's + * on_connection_established() method. + * + * @param addr Network address of the remote client (IPv4/IPv6 compatible) + * @param fd File descriptor for the socket connection + * @param id Unique connection identifier generated by the server + * @param handler Protocol handler for processing connection data + * @param server Weak reference to the server for safe cleanup + */ + Connection(struct sockaddr_storage addr, int fd, int64_t id, + ConnectionHandler *handler, std::weak_ptr server); + + /** + * @brief Server-only factory method for creating connections. + * + * This factory method can only be called by the Server class due to friend + * access. It provides controlled connection creation with proper resource + * management. + * + * @param addr Network address of the remote client (IPv4/IPv6 compatible) + * @param fd File descriptor for the socket connection + * @param id Unique connection identifier generated by the server + * @param handler Protocol handler for processing connection data + * @param server Weak reference to the server for safe cleanup + * + * @return std::unique_ptr to the newly created connection + * + * @note This method is only accessible to the Server class and should be used + * exclusively by accept threads when new connections arrive. + */ + static std::unique_ptr + createForServer(struct sockaddr_storage addr, int fd, int64_t id, + ConnectionHandler *handler, std::weak_ptr server); + // Networking interface - only accessible by Server std::string_view readBytes(size_t max_request_size, size_t buffer_size); bool writeBytes(); diff --git a/src/server.cpp b/src/server.cpp index 2122845..fc29bba 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -391,7 +391,7 @@ void Server::start_accept_threads() { perror("setsockopt SO_KEEPALIVE"); } - auto conn = std::make_unique( + auto conn = Connection::createForServer( addr, fd, connection_id_.fetch_add(1, std::memory_order_relaxed), &handler_, weak_from_this());