From 8682bbff3041bffc294ad91d8c2130a8e2cc020e Mon Sep 17 00:00:00 2001 From: David Cozens Date: Wed, 6 May 2026 21:16:49 +0100 Subject: [PATCH 01/16] =?UTF-8?q?feat:=20S12.14=20eager=20Service=20drain?= =?UTF-8?q?=20=E2=80=94=20buffer=E2=86=92store=20decoupled=20from=20sender?= =?UTF-8?q?=20state?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ProcessMessages now drains the buffer eagerly into the store and then attempts one send from the store. When a write does not land in the store (NullStore, or store-write rejection), it falls back to a best-effort direct send to preserve the existing single-task / no-store-and-forward configurations. StoreFake promoted to FIFO (8 slots) with a write counter for the burst-drain assertion. New Service tests use the production CircularBuffer + NullMutex. --- Core/Source/SolidSyslog.c | 53 ++++++++++++++++++----------------- Tests/SolidSyslogTest.cpp | 59 +++++++++++++++++++++++++++++++++++++++ Tests/StoreFake.c | 55 ++++++++++++++++++++++++------------ Tests/StoreFake.h | 1 + Tests/StoreFakeTest.cpp | 34 ++++++++++++++++++++++ 5 files changed, 159 insertions(+), 43 deletions(-) diff --git a/Core/Source/SolidSyslog.c b/Core/Source/SolidSyslog.c index 9d96c7da..9e84595d 100644 --- a/Core/Source/SolidSyslog.c +++ b/Core/Source/SolidSyslog.c @@ -29,7 +29,9 @@ static inline int16_t AbsoluteInt16(int16_t value); static inline bool CaptureTimestamp(struct SolidSyslogTimestamp* ts, SolidSyslogClockFunction clock); static inline uint8_t CombineFacilityAndSeverity(uint8_t facility, uint8_t severity); static inline bool FacilityIsValid(uint8_t facility); -static inline bool FetchFromStore(char* buf, size_t maxSize, size_t* len); +static inline void DrainBufferIntoStore(void); +static inline bool StoreDidNotRetainLastWrite(void); +static inline void SendOneFromStore(void); static inline void FormatCapturedTimestamp(struct SolidSyslogFormatter* f, const struct SolidSyslogTimestamp* ts); static inline void FormatMessage(struct SolidSyslogFormatter* f, const struct SolidSyslogMessage* message); static inline void FormatMsg(struct SolidSyslogFormatter* f, const char* msg); @@ -47,7 +49,6 @@ static void NilClock(struct SolidSyslogTimestamp* ts); static void NilStringFunction(struct SolidSyslogFormatter* formatter); static inline bool PrivalComponentsAreValid(uint8_t facility, uint8_t severity); static void ProcessMessages(void); -static inline bool ReceiveFromBufferIntoStore(char* buf, size_t maxSize, size_t* len); static inline bool SeverityIsValid(uint8_t severity); static inline const char* SkipLeadingBom(const char* msg); static inline bool StringIsValid(const char* value); @@ -123,45 +124,47 @@ static inline bool IsServiceEnabled(void) } static void ProcessMessages(void) +{ + DrainBufferIntoStore(); + SendOneFromStore(); +} + +/* Eagerly drain the buffer so the producer-side shock absorber stays small while + * the sender is slow or down — overflow then engages the store's discard policy + * rather than silently dropping at the buffer. When a write does not land in the + * store (NullStore configuration, or a store-write rejection), best-effort + * direct send keeps the message moving. */ +static inline void DrainBufferIntoStore(void) { char buf[SOLIDSYSLOG_MAX_MESSAGE_SIZE]; size_t len = 0; - bool haveMessage = ReceiveFromBufferIntoStore(buf, sizeof(buf), &len); - bool fromStore = FetchFromStore(buf, sizeof(buf), &len); - - if (fromStore || haveMessage) + while (SolidSyslogBuffer_Read(instance.buffer, buf, sizeof(buf), &len)) { - if (SolidSyslogSender_Send(instance.sender, buf, len)) + SolidSyslogStore_Write(instance.store, buf, len); + + if (StoreDidNotRetainLastWrite()) { - if (fromStore) - { - SolidSyslogStore_MarkSent(instance.store); - } + SolidSyslogSender_Send(instance.sender, buf, len); } } } -static inline bool ReceiveFromBufferIntoStore(char* buf, size_t maxSize, size_t* len) +static inline bool StoreDidNotRetainLastWrite(void) { - bool received = SolidSyslogBuffer_Read(instance.buffer, buf, maxSize, len); - - if (received) - { - SolidSyslogStore_Write(instance.store, buf, *len); - } - - return received; + return !SolidSyslogStore_HasUnsent(instance.store); } -static inline bool FetchFromStore(char* buf, size_t maxSize, size_t* len) +static inline void SendOneFromStore(void) { - if (SolidSyslogStore_HasUnsent(instance.store)) + char buf[SOLIDSYSLOG_MAX_MESSAGE_SIZE]; + size_t len = 0; + + if (SolidSyslogStore_HasUnsent(instance.store) && SolidSyslogStore_ReadNextUnsent(instance.store, buf, sizeof(buf), &len) && + SolidSyslogSender_Send(instance.sender, buf, len)) { - return SolidSyslogStore_ReadNextUnsent(instance.store, buf, maxSize, len); + SolidSyslogStore_MarkSent(instance.store); } - - return false; } void SolidSyslog_Log(const struct SolidSyslogMessage* message) diff --git a/Tests/SolidSyslogTest.cpp b/Tests/SolidSyslogTest.cpp index 4c1d9d41..d4f38d39 100644 --- a/Tests/SolidSyslogTest.cpp +++ b/Tests/SolidSyslogTest.cpp @@ -8,7 +8,9 @@ #include "SolidSyslogMetaSd.h" #include "TestAtomicOps.h" #include "SolidSyslogTimeQualitySd.h" +#include "SolidSyslogCircularBuffer.h" #include "SolidSyslogNullBuffer.h" +#include "SolidSyslogNullMutex.h" #include "SolidSyslogNullStore.h" #include "SolidSyslogFormatter.h" #include "SolidSyslogStructuredDataDefinition.h" @@ -1471,6 +1473,63 @@ TEST(SolidSyslog, ServiceDoesNotMarkSentWhenSendingFromBuffer) BufferFake_Destroy(); } +TEST(SolidSyslog, ServiceEagerlyDrainsBufferIntoStoreInOneTick) +{ + static constexpr size_t BUFFER_BYTES = 256; + SolidSyslogCircularBufferStorage bufferStorage[SOLIDSYSLOG_CIRCULARBUFFER_STORAGE_SIZE_BYTES(BUFFER_BYTES)]; + SolidSyslogBuffer* circularBuffer = SolidSyslogCircularBuffer_Create(bufferStorage, sizeof(bufferStorage), SolidSyslogNullMutex_Create()); + SolidSyslogStore* fakeStore = StoreFake_Create(); + SolidSyslogConfig serviceConfig = {circularBuffer, fakeSender, nullptr, nullptr, nullptr, nullptr, fakeStore, nullptr, 0}; + + SolidSyslog_Destroy(); + SolidSyslog_Create(&serviceConfig); + + SolidSyslogBuffer_Write(circularBuffer, "msg1", 4); + SolidSyslogBuffer_Write(circularBuffer, "msg2", 4); + SolidSyslogBuffer_Write(circularBuffer, "msg3", 4); + SenderFake_FailNextSend(fakeSender); + SolidSyslog_Service(); + + LONGS_EQUAL(3, StoreFake_WriteCount(fakeStore)); + + SolidSyslog_Destroy(); + SolidSyslog_Create(&config); + StoreFake_Destroy(); + SolidSyslogCircularBuffer_Destroy(circularBuffer); + SolidSyslogNullMutex_Destroy(); +} + +TEST(SolidSyslog, ServiceSendsStoredMessagesInFifoOrderAcrossTicks) +{ + static constexpr size_t BUFFER_BYTES = 256; + SolidSyslogCircularBufferStorage bufferStorage[SOLIDSYSLOG_CIRCULARBUFFER_STORAGE_SIZE_BYTES(BUFFER_BYTES)]; + SolidSyslogBuffer* circularBuffer = SolidSyslogCircularBuffer_Create(bufferStorage, sizeof(bufferStorage), SolidSyslogNullMutex_Create()); + SolidSyslogStore* fakeStore = StoreFake_Create(); + SolidSyslogConfig serviceConfig = {circularBuffer, fakeSender, nullptr, nullptr, nullptr, nullptr, fakeStore, nullptr, 0}; + + SolidSyslog_Destroy(); + SolidSyslog_Create(&serviceConfig); + + SolidSyslogBuffer_Write(circularBuffer, "m1", 2); + SolidSyslogBuffer_Write(circularBuffer, "m2", 2); + SolidSyslogBuffer_Write(circularBuffer, "m3", 2); + + SenderFake_Reset(fakeSender); + SolidSyslog_Service(); + STRCMP_EQUAL("m1", SenderFake_LastBufferAsString(fakeSender)); + SolidSyslog_Service(); + STRCMP_EQUAL("m2", SenderFake_LastBufferAsString(fakeSender)); + SolidSyslog_Service(); + STRCMP_EQUAL("m3", SenderFake_LastBufferAsString(fakeSender)); + LONGS_EQUAL(3, SenderFake_SendCount(fakeSender)); + + SolidSyslog_Destroy(); + SolidSyslog_Create(&config); + StoreFake_Destroy(); + SolidSyslogCircularBuffer_Destroy(circularBuffer); + SolidSyslogNullMutex_Destroy(); +} + TEST(SolidSyslog, ServiceDoesNothingWhenStoreIsHalted) { SolidSyslogBuffer* fakeBuffer = BufferFake_Create(); diff --git a/Tests/StoreFake.c b/Tests/StoreFake.c index 2eeae5d0..5ac08605 100644 --- a/Tests/StoreFake.c +++ b/Tests/StoreFake.c @@ -8,7 +8,8 @@ enum { - STOREFAKE_MAX_SIZE = 1024 + STOREFAKE_MAX_MESSAGES = 8, + STOREFAKE_MAX_MESSAGE_SIZE = 1024 }; static bool Write(struct SolidSyslogStore* self, const void* data, size_t size); @@ -20,9 +21,10 @@ static bool IsHalted(struct SolidSyslogStore* self); struct StoreFake { struct SolidSyslogStore base; - char stored[STOREFAKE_MAX_SIZE]; - size_t storedSize; - bool unsent; + char entries[STOREFAKE_MAX_MESSAGES][STOREFAKE_MAX_MESSAGE_SIZE]; + size_t sizes[STOREFAKE_MAX_MESSAGES]; + size_t count; + int writeCount; bool failNextWrite; bool failNextRead; bool halted; @@ -56,6 +58,11 @@ void StoreFake_FailNextRead(void) instance.failNextRead = true; } +int StoreFake_WriteCount(struct SolidSyslogStore* store) +{ + return ((struct StoreFake*) store)->writeCount; +} + static bool Write(struct SolidSyslogStore* self, const void* data, size_t size) { struct StoreFake* fake = (struct StoreFake*) self; @@ -66,13 +73,15 @@ static bool Write(struct SolidSyslogStore* self, const void* data, size_t size) return false; } - if (!fake->unsent) + if (fake->count < STOREFAKE_MAX_MESSAGES) { - size_t copySize = MinSize(size, sizeof(fake->stored)); - memcpy(fake->stored, data, copySize); - fake->storedSize = copySize; - fake->unsent = true; + size_t copySize = MinSize(size, STOREFAKE_MAX_MESSAGE_SIZE); + memcpy(fake->entries[fake->count], data, copySize); + fake->sizes[fake->count] = copySize; + fake->count++; } + + fake->writeCount++; return true; } @@ -87,28 +96,38 @@ static bool ReadNextUnsent(struct SolidSyslogStore* self, void* data, size_t max return false; } - bool success = fake->unsent; - - if (success) + if (fake->count == 0) { - size_t copySize = MinSize(fake->storedSize, maxSize); - memcpy(data, fake->stored, copySize); - *bytesRead = copySize; + return false; } - return success; + size_t copySize = MinSize(fake->sizes[0], maxSize); + memcpy(data, fake->entries[0], copySize); + *bytesRead = copySize; + return true; } static void MarkSent(struct SolidSyslogStore* self) { struct StoreFake* fake = (struct StoreFake*) self; - fake->unsent = false; + + if (fake->count == 0) + { + return; + } + + for (size_t i = 1; i < fake->count; i++) + { + memcpy(fake->entries[i - 1], fake->entries[i], fake->sizes[i]); + fake->sizes[i - 1] = fake->sizes[i]; + } + fake->count--; } static bool HasUnsent(struct SolidSyslogStore* self) { struct StoreFake* fake = (struct StoreFake*) self; - return fake->unsent; + return fake->count > 0; } static bool IsHalted(struct SolidSyslogStore* self) diff --git a/Tests/StoreFake.h b/Tests/StoreFake.h index 2b80d63b..dccf5ae2 100644 --- a/Tests/StoreFake.h +++ b/Tests/StoreFake.h @@ -10,6 +10,7 @@ EXTERN_C_BEGIN void StoreFake_FailNextWrite(void); void StoreFake_FailNextRead(void); void StoreFake_SetHalted(void); + int StoreFake_WriteCount(struct SolidSyslogStore * store); EXTERN_C_END diff --git a/Tests/StoreFakeTest.cpp b/Tests/StoreFakeTest.cpp index 86a20b6b..1ac595a0 100644 --- a/Tests/StoreFakeTest.cpp +++ b/Tests/StoreFakeTest.cpp @@ -91,3 +91,37 @@ TEST(StoreFake, ReadCanBeConfiguredToFail) StoreFake_FailNextRead(); CHECK_FALSE(ReadNextUnsent()); } + +TEST(StoreFake, ReadsTwoWrittenMessagesInFifoOrderAcrossMarkSent) +{ + SolidSyslogStore_Write(store, "first", 5); + SolidSyslogStore_Write(store, "second", 6); + + ReadNextUnsent(); + MEMCMP_EQUAL("first", readData, 5); + LONGS_EQUAL(5, readSize); + + SolidSyslogStore_MarkSent(store); + + ReadNextUnsent(); + MEMCMP_EQUAL("second", readData, 6); + LONGS_EQUAL(6, readSize); + + SolidSyslogStore_MarkSent(store); + CHECK_FALSE(SolidSyslogStore_HasUnsent(store)); +} + +TEST(StoreFake, WriteCountReportsSuccessfulWrites) +{ + SolidSyslogStore_Write(store, "a", 1); + SolidSyslogStore_Write(store, "b", 1); + LONGS_EQUAL(2, StoreFake_WriteCount(store)); +} + +TEST(StoreFake, WriteCountIgnoresFailedWrite) +{ + SolidSyslogStore_Write(store, "a", 1); + StoreFake_FailNextWrite(); + SolidSyslogStore_Write(store, "b", 1); + LONGS_EQUAL(1, StoreFake_WriteCount(store)); +} From c5f83eb1d49b86128677eab31c72ad458abffcdd Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 05:45:15 +0100 Subject: [PATCH 02/16] feat: S12.14 PosixTcpStream non-blocking + fail-fast Stream becomes fully non-blocking from socket creation. connect() returns EINPROGRESS and the wait is bounded via select() to 200 ms (mirrors the Winsock pattern); SO_ERROR distinguishes completed-success from deferred- failure. Send is single-call, fail-fast, with internal close on any failure (short write, EAGAIN, errno). Read returns 0 on would-block and -1 with internal close on EOF/error. SolidSyslogStream.h documents the new contract. Drops SO_SNDTIMEO (no-op on non-blocking sockets) and the EINTR retry loop. SocketFake gains fcntl, select, connect-with-errno, recv-with-errno, and getsockopt(SO_ERROR) seams to drive the new code paths under test. --- Core/Interface/SolidSyslogStream.h | 25 +- .../Posix/Source/SolidSyslogPosixTcpStream.c | 140 ++++++--- Tests/SolidSyslogPosixTcpStreamTest.cpp | 178 +++++++++++- Tests/SolidSyslogStreamSenderTest.cpp | 5 +- Tests/Support/SocketFake.c | 273 ++++++++++++++++-- Tests/Support/SocketFake.h | 35 ++- 6 files changed, 585 insertions(+), 71 deletions(-) diff --git a/Core/Interface/SolidSyslogStream.h b/Core/Interface/SolidSyslogStream.h index 18d5cc8d..30c655ae 100644 --- a/Core/Interface/SolidSyslogStream.h +++ b/Core/Interface/SolidSyslogStream.h @@ -12,16 +12,31 @@ struct SolidSyslogAddress; EXTERN_C_BEGIN /* Signed byte count. Models POSIX ssize_t using a standard-C type for portability - to targets that lack 's ssize_t (notably MSVC). Semantics mirror - POSIX recv(2): >=0 = bytes transferred, 0 = EOF on stream reads, <0 = error. */ + to targets that lack 's ssize_t (notably MSVC). */ typedef intptr_t SolidSyslogSsize; struct SolidSyslogStream; - bool SolidSyslogStream_Open(struct SolidSyslogStream * stream, const struct SolidSyslogAddress* addr); - bool SolidSyslogStream_Send(struct SolidSyslogStream * stream, const void* buffer, size_t size); + /* Open a connection to addr. Returns true on success. On failure the underlying + socket has already been closed internally; the caller can call Open again to retry. */ + bool SolidSyslogStream_Open(struct SolidSyslogStream * stream, const struct SolidSyslogAddress* addr); + + /* Non-blocking send. Returns true only when the kernel accepts the entire frame in + a single call. Anything else — short write, EAGAIN/EWOULDBLOCK, EPIPE/ECONNRESET, + any other error — closes the underlying socket internally and returns false; the + caller must call Open before the next Send. Bounded so the service-thread drain + rate stays insensitive to peer wedges or kernel send-buffer fill. */ + bool SolidSyslogStream_Send(struct SolidSyslogStream * stream, const void* buffer, size_t size); + + /* Non-blocking read. + > 0 bytes transferred into buffer + = 0 nothing available right now (would-block) + < 0 EOF or error — the underlying socket has been closed internally; the + caller must call Open before the next Send. */ SolidSyslogSsize SolidSyslogStream_Read(struct SolidSyslogStream * stream, void* buffer, size_t size); - void SolidSyslogStream_Close(struct SolidSyslogStream * stream); + + /* Close the underlying socket. Idempotent. */ + void SolidSyslogStream_Close(struct SolidSyslogStream * stream); EXTERN_C_END diff --git a/Platform/Posix/Source/SolidSyslogPosixTcpStream.c b/Platform/Posix/Source/SolidSyslogPosixTcpStream.c index 57946422..375800b1 100644 --- a/Platform/Posix/Source/SolidSyslogPosixTcpStream.c +++ b/Platform/Posix/Source/SolidSyslogPosixTcpStream.c @@ -1,26 +1,31 @@ #include "SolidSyslogPosixTcpStream.h" #include +#include +#include #include +#include #include +#include #include #include -#include -#include -#include #include +#include #include "SolidSyslogAddressInternal.h" #include "SolidSyslogMacros.h" -#include "SolidSyslogStreamDefinition.h" #include "SolidSyslogStream.h" +#include "SolidSyslogStreamDefinition.h" struct SolidSyslogAddress; enum { - INVALID_FD = -1, - SEND_TIMEOUT_SECONDS = 5 + INVALID_FD = -1, + /* Caps the time a single connect() attempt can stall the service thread. + Mirrors the Winsock value: 200 ms is comfortable for loopback/LAN and + short enough that 10 failing attempts cost 2 s instead of 20+ s. */ + CONNECT_TIMEOUT_MICROSECONDS = 200000 }; struct SolidSyslogPosixTcpStream @@ -35,14 +40,16 @@ static SolidSyslogSsize Read(struct SolidSyslogStream* self, void* buffer, size_ static void Close(struct SolidSyslogStream* self); static int OpenAndConfigureSocket(void); +static bool ConfigureSocket(int fd); static void EnableTcpNoDelay(int fd); -static void SetSendTimeout(int fd); +static bool SetNonBlocking(int fd); static inline bool IsFileDescriptorValid(int fd); static bool ConnectOrCloseOnFailure(struct SolidSyslogPosixTcpStream* stream, const struct sockaddr_in* sin); static bool Connect(int fd, const struct sockaddr_in* sin); -static ssize_t SendRetryingOnSignal(int fd, const void* buffer, size_t size); -static bool WasInterruptedBySignal(void); +static bool WaitForConnectCompletion(int fd); +static bool ReadDeferredConnectError(int fd); static bool WroteAllBytes(ssize_t sent, size_t expected); +static inline bool WouldBlock(void); SOLIDSYSLOG_STATIC_ASSERT(sizeof(struct SolidSyslogPosixTcpStream) <= SOLIDSYSLOG_POSIX_TCP_STREAM_SIZE, "SOLIDSYSLOG_POSIX_TCP_STREAM_SIZE is too small for struct SolidSyslogPosixTcpStream"); @@ -85,33 +92,36 @@ static bool Open(struct SolidSyslogStream* self, const struct SolidSyslogAddress return connected; } +/* Non-blocking from the start: connect() reports EINPROGRESS, the wait is + * bounded by select(), and Send/Read never block the service thread on a + * wedged peer or a full kernel send buffer. */ static int OpenAndConfigureSocket(void) { int fd = socket(AF_INET, SOCK_STREAM, 0); - if (IsFileDescriptorValid(fd)) + if (IsFileDescriptorValid(fd) && !ConfigureSocket(fd)) { - EnableTcpNoDelay(fd); - SetSendTimeout(fd); + close(fd); + fd = INVALID_FD; } return fd; } +static bool ConfigureSocket(int fd) +{ + EnableTcpNoDelay(fd); + return SetNonBlocking(fd); +} + static void EnableTcpNoDelay(int fd) { int enable = 1; setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, &enable, sizeof(enable)); } -/* Caps blocking time of send() so a wedged peer can't make a single - * SolidSyslog_Service() call hang. On expiry, send() returns -1 with - * EAGAIN/EWOULDBLOCK, the Send vtable returns false, and the Service - * loop closes and reconnects on the next attempt; store-and-forward - * replays the message on the fresh socket. Hard-coded for now; - * port-time CMake override is a future option. */ -static void SetSendTimeout(int fd) +static bool SetNonBlocking(int fd) { - struct timeval timeout = {.tv_sec = SEND_TIMEOUT_SECONDS, .tv_usec = 0}; - setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &timeout, sizeof(timeout)); + int flags = fcntl(fd, F_GETFL, 0); + return (flags >= 0) && (fcntl(fd, F_SETFL, flags | O_NONBLOCK) != -1); } static inline bool IsFileDescriptorValid(int fd) @@ -130,37 +140,67 @@ static bool ConnectOrCloseOnFailure(struct SolidSyslogPosixTcpStream* stream, co return connected; } +/* Non-blocking connect with bounded wait. connect() returns immediately: + * 0 — connected (loopback success path). + * -1 EINPROGRESS — connect started; wait via select() up to + * CONNECT_TIMEOUT_MICROSECONDS, then read SO_ERROR to + * distinguish completed-success from deferred-failure. + * -1 other — immediate fail-fast (refused, unreachable, etc.). */ static bool Connect(int fd, const struct sockaddr_in* sin) { - return (connect(fd, (const struct sockaddr*) sin, sizeof(*sin)) == 0); + bool connected = false; + int rc = connect(fd, (const struct sockaddr*) sin, sizeof(*sin)); + + if (rc == 0) + { + connected = true; + } + else if (errno == EINPROGRESS) + { + connected = WaitForConnectCompletion(fd) && ReadDeferredConnectError(fd); + } + return connected; } -static bool Send(struct SolidSyslogStream* self, const void* buffer, size_t size) +static bool WaitForConnectCompletion(int fd) { - struct SolidSyslogPosixTcpStream* stream = (struct SolidSyslogPosixTcpStream*) self; - ssize_t sent = SendRetryingOnSignal(stream->fd, buffer, size); - return WroteAllBytes(sent, size); + fd_set writeSet; + FD_ZERO(&writeSet); + FD_SET(fd, &writeSet); + + fd_set errorSet; + FD_ZERO(&errorSet); + FD_SET(fd, &errorSet); + + struct timeval timeout = {.tv_sec = 0, .tv_usec = CONNECT_TIMEOUT_MICROSECONDS}; + + int rc = select(fd + 1, NULL, &writeSet, &errorSet, &timeout); + return (rc > 0) && FD_ISSET(fd, &writeSet) && !FD_ISSET(fd, &errorSet); } -/* Retry only on EINTR — portability shim for kernels without SA_RESTART. - * Any other failure (including EAGAIN/EWOULDBLOCK from SO_SNDTIMEO) and - * any short return propagate via WroteAllBytes; the caller closes and - * reconnects, store-and-forward replays the message on the fresh socket. */ -static ssize_t SendRetryingOnSignal(int fd, const void* buffer, size_t size) +static bool ReadDeferredConnectError(int fd) { - ssize_t sent = 0; - do - { - sent = send(fd, buffer, size, MSG_NOSIGNAL); - } while ((sent < 0) && WasInterruptedBySignal()); - return sent; + int err = 0; + socklen_t errlen = (socklen_t) sizeof(err); + int rc = getsockopt(fd, SOL_SOCKET, SO_ERROR, &err, &errlen); + return (rc == 0) && (err == 0); } -static bool WasInterruptedBySignal(void) +static bool Send(struct SolidSyslogStream* self, const void* buffer, size_t size) { - return (errno == EINTR); + struct SolidSyslogPosixTcpStream* stream = (struct SolidSyslogPosixTcpStream*) self; + ssize_t sent = send(stream->fd, buffer, size, MSG_NOSIGNAL); + bool ok = WroteAllBytes(sent, size); + + if (!ok) + { + Close(self); + } + return ok; } +/* Non-blocking single-call contract: short write or any error means the + * connection is gone; the caller closes and reconnects on the next attempt. */ static bool WroteAllBytes(ssize_t sent, size_t expected) { return (sent >= 0) && ((size_t) sent == expected); @@ -169,7 +209,27 @@ static bool WroteAllBytes(ssize_t sent, size_t expected) static SolidSyslogSsize Read(struct SolidSyslogStream* self, void* buffer, size_t size) { struct SolidSyslogPosixTcpStream* stream = (struct SolidSyslogPosixTcpStream*) self; - return (SolidSyslogSsize) recv(stream->fd, buffer, size, 0); + ssize_t n = recv(stream->fd, buffer, size, 0); + SolidSyslogSsize result = -1; + + if (n > 0) + { + result = (SolidSyslogSsize) n; + } + else if ((n < 0) && WouldBlock()) + { + result = 0; + } + else + { + Close(self); + } + return result; +} + +static inline bool WouldBlock(void) +{ + return (errno == EAGAIN) || (errno == EWOULDBLOCK); } static void Close(struct SolidSyslogStream* self) diff --git a/Tests/SolidSyslogPosixTcpStreamTest.cpp b/Tests/SolidSyslogPosixTcpStreamTest.cpp index bb0056ba..cc522469 100644 --- a/Tests/SolidSyslogPosixTcpStreamTest.cpp +++ b/Tests/SolidSyslogPosixTcpStreamTest.cpp @@ -88,10 +88,21 @@ TEST(SolidSyslogPosixTcpStream, OpenEnablesTcpNoDelay) CHECK_TRUE(SocketFake_HasSetSockOpt(IPPROTO_TCP, TCP_NODELAY)); } -TEST(SolidSyslogPosixTcpStream, OpenSetsSendTimeout) +TEST(SolidSyslogPosixTcpStream, OpenSetsNonBlockingFlagBeforeConnect) { SolidSyslogStream_Open(stream, addr); - CHECK_TRUE(SocketFake_HasSetSockOpt(SOL_SOCKET, SO_SNDTIMEO)); + /* Non-blocking is set via fcntl(F_SETFL, ... | O_NONBLOCK) so connect() + returns EINPROGRESS immediately and the caller can bound the wait. */ + CHECK_TRUE(SocketFake_FcntlSetFlSetNonBlocking()); +} + +TEST(SolidSyslogPosixTcpStream, OpenFailsWhenFcntlSetFlFails) +{ + /* If the kernel refuses to put the socket into non-blocking mode the + caller cannot bound the connect wait — fail fast. */ + SocketFake_SetFcntlSetFlFails(true); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); + LONGS_EQUAL(1, SocketFake_CloseCallCount()); } TEST(SolidSyslogPosixTcpStream, OpenCallsConnectWithSocketFd) @@ -148,20 +159,32 @@ TEST(SolidSyslogPosixTcpStream, SendDoesNotRetryAfterShortWrite) LONGS_EQUAL(1, SocketFake_SendCallCount()); } -TEST(SolidSyslogPosixTcpStream, SendRetriesOnEintrAndSucceeds) +TEST(SolidSyslogPosixTcpStream, SendReturnsFalseOnEintr) { + /* Non-blocking + fail-fast: any failure from a single send() call closes + the socket and surfaces failure. EINTR is no longer retried. */ SolidSyslogStream_Open(stream, addr); SocketFake_FailNextSendWithErrno(EINTR); - CHECK_TRUE(SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN)); + CHECK_FALSE(SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN)); } -TEST(SolidSyslogPosixTcpStream, SendReturnsFalseOnEagainTimeout) +TEST(SolidSyslogPosixTcpStream, SendReturnsFalseOnEagain) { SolidSyslogStream_Open(stream, addr); SocketFake_FailNextSendWithErrno(EAGAIN); CHECK_FALSE(SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN)); } +TEST(SolidSyslogPosixTcpStream, SendClosesSocketOnFailure) +{ + SolidSyslogStream_Open(stream, addr); + int openFd = SocketFake_SocketFd(); + SocketFake_SetSendFails(true); + SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN); + LONGS_EQUAL(1, SocketFake_CloseCallCount()); + LONGS_EQUAL(openFd, SocketFake_LastClosedFd()); +} + TEST(SolidSyslogPosixTcpStream, OpenClosesSocketOnConnectFailure) { SocketFake_SetConnectFails(true); @@ -305,3 +328,148 @@ TEST(SolidSyslogPosixTcpStream, DefaultPortMatchesRfc6587) { LONGS_EQUAL(601, SOLIDSYSLOG_TCP_DEFAULT_PORT); } + +/* ---------------------------------------------------------------------- + * Non-blocking connect with bounded wait — keeps the service-thread drain + * rate insensitive to a slow or refused peer. + * -------------------------------------------------------------------- */ + +TEST(SolidSyslogPosixTcpStream, OpenSkipsSelectWhenConnectReturnsImmediately) +{ + /* Default fake connect returns 0 (immediate success); select must not be + reached because connect short-circuits the wait. */ + SolidSyslogStream_Open(stream, addr); + LONGS_EQUAL(0, SocketFake_SelectCallCount()); +} + +TEST(SolidSyslogPosixTcpStream, OpenInvokesSelectWhenConnectReturnsEinprogress) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SolidSyslogStream_Open(stream, addr); + LONGS_EQUAL(1, SocketFake_SelectCallCount()); +} + +TEST(SolidSyslogPosixTcpStream, OpenPassesBoundedConnectTimeoutToSelect) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SolidSyslogStream_Open(stream, addr); + /* CONNECT_TIMEOUT_MICROSECONDS = 200 000 → 0 s + 200 000 µs. */ + LONGS_EQUAL(0, SocketFake_LastSelectTimeoutSec()); + LONGS_EQUAL(200000, SocketFake_LastSelectTimeoutUsec()); +} + +TEST(SolidSyslogPosixTcpStream, OpenSucceedsWhenSelectReportsWritableAndZeroSO_ERROR) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SocketFake_SetSelectWritable(true); + SocketFake_SetSoError(0); + CHECK_TRUE(SolidSyslogStream_Open(stream, addr)); +} + +TEST(SolidSyslogPosixTcpStream, OpenFailsWhenSelectReportsTimeout) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SocketFake_SetSelectWritable(false); + SocketFake_SetSelectReturn(0); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); +} + +TEST(SolidSyslogPosixTcpStream, OpenClosesSocketOnSelectTimeout) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SocketFake_SetSelectWritable(false); + SocketFake_SetSelectReturn(0); + SolidSyslogStream_Open(stream, addr); + LONGS_EQUAL(1, SocketFake_CloseCallCount()); + LONGS_EQUAL(SocketFake_SocketFd(), SocketFake_LastClosedFd()); +} + +TEST(SolidSyslogPosixTcpStream, OpenFailsWhenSelectFlagsErrorOnFd) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SocketFake_SetSelectWritable(false); + SocketFake_SetSelectError(true); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); +} + +TEST(SolidSyslogPosixTcpStream, OpenFailsWhenDeferredSO_ERRORIsNonZero) +{ + /* select reports writable, but SO_ERROR on the socket reveals the + deferred connect failure (e.g. ECONNREFUSED). */ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SocketFake_SetSelectWritable(true); + SocketFake_SetSoError(ECONNREFUSED); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); +} + +TEST(SolidSyslogPosixTcpStream, OpenReadsSO_ERRORAfterSelectWritable) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SolidSyslogStream_Open(stream, addr); + LONGS_EQUAL(SOL_SOCKET, SocketFake_LastGetSockOptLevel()); + LONGS_EQUAL(SO_ERROR, SocketFake_LastGetSockOptOptname()); +} + +TEST(SolidSyslogPosixTcpStream, OpenFailsWhenConnectFailsImmediatelyWithRefused) +{ + /* Non-EINPROGRESS errors are immediate failures — no select wait, no + SO_ERROR check, just fail fast. */ + SocketFake_SetConnectFailsWithErrno(ECONNREFUSED); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); + LONGS_EQUAL(0, SocketFake_SelectCallCount()); +} + +TEST(SolidSyslogPosixTcpStream, OpenFailsWhenSO_ERRORLookupFails) +{ + SocketFake_SetConnectFailsWithErrno(EINPROGRESS); + SocketFake_SetSelectWritable(true); + SocketFake_SetSoErrorLookupFails(true); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); +} + +/* ---------------------------------------------------------------------- + * Non-blocking Read contract: bytes → return them; nothing → 0; + * EOF/error → close internally and return -1. + * -------------------------------------------------------------------- */ + +TEST(SolidSyslogPosixTcpStream, ReadReturnsZeroOnEagain) +{ + SolidSyslogStream_Open(stream, addr); + SocketFake_FailNextRecvWithErrno(EAGAIN); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(0, n); +} + +TEST(SolidSyslogPosixTcpStream, ReadReturnsZeroOnWouldBlock) +{ + SolidSyslogStream_Open(stream, addr); + SocketFake_FailNextRecvWithErrno(EWOULDBLOCK); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(0, n); +} + +TEST(SolidSyslogPosixTcpStream, ReadReturnsNegativeOneOnEofAndClosesSocket) +{ + SolidSyslogStream_Open(stream, addr); + int openFd = SocketFake_SocketFd(); + SocketFake_SetRecvReturn(0); /* EOF */ + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(-1, n); + LONGS_EQUAL(1, SocketFake_CloseCallCount()); + LONGS_EQUAL(openFd, SocketFake_LastClosedFd()); +} + +TEST(SolidSyslogPosixTcpStream, ReadReturnsNegativeOneOnErrorAndClosesSocket) +{ + SolidSyslogStream_Open(stream, addr); + int openFd = SocketFake_SocketFd(); + SocketFake_FailNextRecvWithErrno(ECONNRESET); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(-1, n); + LONGS_EQUAL(1, SocketFake_CloseCallCount()); + LONGS_EQUAL(openFd, SocketFake_LastClosedFd()); +} diff --git a/Tests/SolidSyslogStreamSenderTest.cpp b/Tests/SolidSyslogStreamSenderTest.cpp index c56419f7..1c138915 100644 --- a/Tests/SolidSyslogStreamSenderTest.cpp +++ b/Tests/SolidSyslogStreamSenderTest.cpp @@ -539,9 +539,8 @@ TEST(SolidSyslogStreamSenderFailure, ReconnectSetsTcpNoDelay) Send(); SocketFake_SetSendFails(false); Send(); - /* Two opens (initial + reconnect); both must set TCP_NODELAY. - * Counted indirectly: TCP_NODELAY appears twice, plus SO_SNDTIMEO twice = 4 total. */ - LONGS_EQUAL(4, SocketFake_SetSockOptCallCount()); + /* Two opens (initial + reconnect); both must set TCP_NODELAY. */ + LONGS_EQUAL(2, SocketFake_SetSockOptCallCount()); CHECK_TRUE(SocketFake_HasSetSockOpt(IPPROTO_TCP, TCP_NODELAY)); } diff --git a/Tests/Support/SocketFake.c b/Tests/Support/SocketFake.c index 4efb13c3..6b844d25 100644 --- a/Tests/Support/SocketFake.c +++ b/Tests/Support/SocketFake.c @@ -3,10 +3,16 @@ #include "SolidSyslog.h" #include #include +#include #include #include +#include #include +#include #include +#include + +struct timeval; enum { @@ -27,6 +33,30 @@ static int lastSendtoFd; static int ipMtuValue; static bool ipMtuLookupFails; static int getSockOptCallCount; +static int lastGetSockOptLevel; +static int lastGetSockOptOptname; +static int soErrorValue; +static bool soErrorLookupFails; + +static int fcntlCallCount; +static int lastFcntlCmd; +static int lastFcntlSetFlags; +static int fcntlGetFlReturn; +static bool fcntlSetFlFails; + +static int selectCallCount; +static bool selectReady; +static bool selectError; +static bool selectReturnOverride; +static int selectReturnValue; +static long lastSelectTimeoutSec; +static long lastSelectTimeoutUsec; + +static bool nextConnectShouldFailWithErrno; +static int nextConnectErrno; + +static bool nextRecvShouldFailWithErrno; +static int nextRecvErrno; static bool socketFails; static int socketCallCount; @@ -130,23 +160,47 @@ void SocketFake_Reset(void) setSockOptLevels[i] = 0; setSockOptOptnames[i] = 0; } - getSockOptCallCount = 0; - ipMtuValue = 0; - ipMtuLookupFails = false; - socketFails = false; - socketCallCount = 0; - socketFd = -1; - lastSocketDomain = 0; - lastSocketType = 0; - closeCallCount = 0; - lastClosedFd = -1; - recvCallCount = 0; - recvReturn = 0; - lastRecvFd = -1; - lastRecvBuf = NULL; - lastRecvLen = 0; - lastRecvFlags = 0; - lastAddrString[0] = '\0'; + getSockOptCallCount = 0; + lastGetSockOptLevel = 0; + lastGetSockOptOptname = 0; + ipMtuValue = 0; + ipMtuLookupFails = false; + soErrorValue = 0; + soErrorLookupFails = false; + + fcntlCallCount = 0; + lastFcntlCmd = 0; + lastFcntlSetFlags = 0; + fcntlGetFlReturn = 0; + fcntlSetFlFails = false; + + selectCallCount = 0; + selectReady = true; + selectError = false; + selectReturnOverride = false; + selectReturnValue = 0; + lastSelectTimeoutSec = 0; + lastSelectTimeoutUsec = 0; + + nextConnectShouldFailWithErrno = false; + nextConnectErrno = 0; + + nextRecvShouldFailWithErrno = false; + nextRecvErrno = 0; + socketFails = false; + socketCallCount = 0; + socketFd = -1; + lastSocketDomain = 0; + lastSocketType = 0; + closeCallCount = 0; + lastClosedFd = -1; + recvCallCount = 0; + recvReturn = 0; + lastRecvFd = -1; + lastRecvBuf = NULL; + lastRecvLen = 0; + lastRecvFlags = 0; + lastAddrString[0] = '\0'; getAddrInfoFails = false; getAddrInfoCallCount = 0; @@ -177,6 +231,99 @@ void SocketFake_SetIpMtu(int mtu) ipMtuValue = mtu; } +void SocketFake_SetSoError(int err) +{ + soErrorValue = err; +} + +void SocketFake_SetSoErrorLookupFails(bool fails) +{ + soErrorLookupFails = fails; +} + +int SocketFake_LastGetSockOptLevel(void) +{ + return lastGetSockOptLevel; +} + +int SocketFake_LastGetSockOptOptname(void) +{ + return lastGetSockOptOptname; +} + +void SocketFake_SetConnectFailsWithErrno(int errnoValue) +{ + nextConnectShouldFailWithErrno = true; + nextConnectErrno = errnoValue; +} + +void SocketFake_SetFcntlSetFlFails(bool fails) +{ + fcntlSetFlFails = fails; +} + +void SocketFake_SetFcntlGetFlReturn(int flags) +{ + fcntlGetFlReturn = flags; +} + +int SocketFake_FcntlCallCount(void) +{ + return fcntlCallCount; +} + +int SocketFake_LastFcntlCmd(void) +{ + return lastFcntlCmd; +} + +int SocketFake_LastFcntlSetFlags(void) +{ + return lastFcntlSetFlags; +} + +bool SocketFake_FcntlSetFlSetNonBlocking(void) +{ + return (lastFcntlSetFlags & O_NONBLOCK) != 0; +} + +void SocketFake_SetSelectWritable(bool ready) +{ + selectReady = ready; +} + +void SocketFake_SetSelectError(bool hasError) +{ + selectError = hasError; +} + +void SocketFake_SetSelectReturn(int value) +{ + selectReturnOverride = true; + selectReturnValue = value; +} + +int SocketFake_SelectCallCount(void) +{ + return selectCallCount; +} + +long SocketFake_LastSelectTimeoutSec(void) +{ + return lastSelectTimeoutSec; +} + +long SocketFake_LastSelectTimeoutUsec(void) +{ + return lastSelectTimeoutUsec; +} + +void SocketFake_FailNextRecvWithErrno(int errnoValue) +{ + nextRecvShouldFailWithErrno = true; + nextRecvErrno = errnoValue; +} + void SocketFake_SetIpMtuLookupFails(bool fails) { ipMtuLookupFails = fails; @@ -527,6 +674,8 @@ int getsockopt(int sockfd, int level, int optname, void* optval, socklen_t* optl { (void) sockfd; getSockOptCallCount++; + lastGetSockOptLevel = level; + lastGetSockOptOptname = optname; if ((level == IPPROTO_IP) && (optname == IP_MTU)) { if (ipMtuLookupFails) @@ -540,6 +689,19 @@ int getsockopt(int sockfd, int level, int optname, void* optval, socklen_t* optl } return 0; } + if ((level == SOL_SOCKET) && (optname == SO_ERROR)) + { + if (soErrorLookupFails) + { + return -1; + } + if ((optval != NULL) && (optlen != NULL) && (*optlen >= sizeof(int))) + { + *(int*) optval = soErrorValue; + *optlen = sizeof(int); + } + return 0; + } return 0; } @@ -586,6 +748,12 @@ int connect(int sockfd, const struct sockaddr* addr, socklen_t addrlen) connectCallCount++; lastConnectFd = sockfd; lastConnectAddr = *(const struct sockaddr_in*) addr; + if (nextConnectShouldFailWithErrno) + { + errno = nextConnectErrno; + nextConnectShouldFailWithErrno = false; + return -1; + } return connectFails ? -1 : 0; } @@ -625,9 +793,80 @@ ssize_t recv(int sockfd, void* buf, size_t len, int flags) lastRecvBuf = buf; lastRecvLen = len; lastRecvFlags = flags; + if (nextRecvShouldFailWithErrno) + { + errno = nextRecvErrno; + nextRecvShouldFailWithErrno = false; + return (ssize_t) -1; + } return recvReturn; } +/* fcntl is variadic in glibc; its third argument is an int when cmd is F_GETFL/ + * F_SETFL (the only commands the production code uses). */ +// NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name) -- POSIX API +int fcntl(int fd, int cmd, ...) +{ + (void) fd; + fcntlCallCount++; + lastFcntlCmd = cmd; + if (cmd == F_GETFL) + { + return fcntlGetFlReturn; + } + if (cmd == F_SETFL) + { + va_list ap; + va_start(ap, cmd); + lastFcntlSetFlags = va_arg(ap, int); + va_end(ap); + return fcntlSetFlFails ? -1 : 0; + } + return 0; +} + +// clang-format off +// NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name,bugprone-easily-swappable-parameters) -- POSIX API +int select(int nfds, fd_set* readfds, fd_set* writefds, fd_set* exceptfds, struct timeval* timeout) +// clang-format on +{ + (void) nfds; + (void) readfds; + selectCallCount++; + if (timeout != NULL) + { + lastSelectTimeoutSec = timeout->tv_sec; + lastSelectTimeoutUsec = timeout->tv_usec; + } + if (writefds != NULL) + { + if (selectReady) + { + FD_SET(nfds - 1, writefds); + } + else + { + FD_ZERO(writefds); + } + } + if (exceptfds != NULL) + { + if (selectError) + { + FD_SET(nfds - 1, exceptfds); + } + else + { + FD_ZERO(exceptfds); + } + } + if (selectReturnOverride) + { + return selectReturnValue; + } + return selectReady ? 1 : 0; +} + // clang-format off // NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name,bugprone-easily-swappable-parameters) -- POSIX API; parameter names differ from glibc internal names int getaddrinfo(const char* node, const char* service, const struct addrinfo* hints, struct addrinfo** res) diff --git a/Tests/Support/SocketFake.h b/Tests/Support/SocketFake.h index d78af8cd..e16518e0 100644 --- a/Tests/Support/SocketFake.h +++ b/Tests/Support/SocketFake.h @@ -52,6 +52,9 @@ EXTERN_C_BEGIN /* connect configuration */ void SocketFake_SetConnectFails(bool fails); + /* When set, connect returns -1 with errno == errnoValue (e.g. EINPROGRESS so + the non-blocking-connect path can be exercised). One-shot. */ + void SocketFake_SetConnectFailsWithErrno(int errnoValue); /* connect accessors */ int SocketFake_ConnectCallCount(void); @@ -65,12 +68,41 @@ EXTERN_C_BEGIN int SocketFake_LastSetSockOptOptname(void); bool SocketFake_HasSetSockOpt(int level, int optname); - /* getsockopt configuration (currently models IPPROTO_IP / IP_MTU only) */ + /* getsockopt configuration (models IPPROTO_IP / IP_MTU and SOL_SOCKET / SO_ERROR) */ void SocketFake_SetIpMtu(int mtu); void SocketFake_SetIpMtuLookupFails(bool fails); + /* SOL_SOCKET / SO_ERROR — read by the non-blocking-connect completion path. + Defaults to 0 (success) until set. */ + void SocketFake_SetSoError(int err); + void SocketFake_SetSoErrorLookupFails(bool fails); /* getsockopt accessors */ int SocketFake_GetSockOptCallCount(void); + int SocketFake_LastGetSockOptLevel(void); + int SocketFake_LastGetSockOptOptname(void); + + /* fcntl configuration */ + void SocketFake_SetFcntlSetFlFails(bool fails); + void SocketFake_SetFcntlGetFlReturn(int flags); + + /* fcntl accessors */ + int SocketFake_FcntlCallCount(void); + int SocketFake_LastFcntlCmd(void); + int SocketFake_LastFcntlSetFlags(void); + bool SocketFake_FcntlSetFlSetNonBlocking(void); + + /* select configuration. ready=true makes select report fd writable in writefds; + ready=false leaves writefds empty (timeout). hasError=true sets fd in + exceptfds. returnValue overrides the int return (1 ready, 0 timeout, -1 error). + Default: returns 1 (one fd ready: writable, no error). */ + void SocketFake_SetSelectWritable(bool ready); + void SocketFake_SetSelectError(bool hasError); + void SocketFake_SetSelectReturn(int value); + + /* select accessors */ + int SocketFake_SelectCallCount(void); + long SocketFake_LastSelectTimeoutSec(void); + long SocketFake_LastSelectTimeoutUsec(void); /* close accessors */ int SocketFake_CloseCallCount(void); @@ -78,6 +110,7 @@ EXTERN_C_BEGIN /* recv configuration */ void SocketFake_SetRecvReturn(ssize_t value); + void SocketFake_FailNextRecvWithErrno(int errnoValue); /* recv accessors */ int SocketFake_RecvCallCount(void); From 33bc03ef90fee42abb2f242e391c08e4f7bf3eb7 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 06:00:57 +0100 Subject: [PATCH 03/16] feat: S12.14 WinsockTcpStream Send/Read non-blocking + fail-fast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The socket is now non-blocking from creation through the full lifecycle — no longer flips back to blocking after a successful connect. Send returns true only when the kernel accepts the entire frame in one call; any failure (short write, WSAEWOULDBLOCK, error) closes the socket internally and returns false. Read returns 0 on WSAEWOULDBLOCK and -1 with internal close on EOF (recv == 0) or other errors. Drops SO_SNDTIMEO (no-op on non-blocking sockets). WinsockFake gains FailNextRecvWithLastError so tests can drive the would-block / error / EOF Read paths. Validated locally on MSVC: 956 tests pass, including the 50 Winsock TCP stream tests that exercise the new non-blocking Send / Read contract. --- .../Source/SolidSyslogWinsockTcpStream.c | 134 +++++++++--------- Tests/SolidSyslogWinsockTcpStreamTest.cpp | 64 ++++++--- Tests/Support/WinsockFake.c | 28 +++- Tests/Support/WinsockFake.h | 3 + 4 files changed, 142 insertions(+), 87 deletions(-) diff --git a/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c b/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c index a103a255..23fff8ce 100644 --- a/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c +++ b/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c @@ -21,16 +21,16 @@ static int WSAAPI CallIoctlSocket(SOCKET s, long cmd, u_long* argp); static int WSAAPI CallSelect(int nfds, fd_set* readfds, fd_set* writefds, fd_set* exceptfds, const struct timeval* timeout); static int WSAAPI CallWSAGetLastError(void); -WinsockTcpStreamSocketFn WinsockTcpStream_socket = CallSocket; -WinsockTcpStreamConnectFn WinsockTcpStream_connect = CallConnect; -WinsockTcpStreamSendFn WinsockTcpStream_send = CallSend; -WinsockTcpStreamRecvFn WinsockTcpStream_recv = CallRecv; -WinsockTcpStreamSetSockOptFn WinsockTcpStream_setsockopt = CallSetSockOpt; -WinsockTcpStreamGetSockOptFn WinsockTcpStream_getsockopt = CallGetSockOpt; -WinsockTcpStreamCloseSocketFn WinsockTcpStream_closesocket = CallCloseSocket; -WinsockTcpStreamIoctlSocketFn WinsockTcpStream_ioctlsocket = CallIoctlSocket; -WinsockTcpStreamSelectFn WinsockTcpStream_select = CallSelect; -WinsockTcpStreamWSAGetLastErrorFn WinsockTcpStream_WSAGetLastError = CallWSAGetLastError; +WinsockTcpStreamSocketFn WinsockTcpStream_socket = CallSocket; +WinsockTcpStreamConnectFn WinsockTcpStream_connect = CallConnect; +WinsockTcpStreamSendFn WinsockTcpStream_send = CallSend; +WinsockTcpStreamRecvFn WinsockTcpStream_recv = CallRecv; +WinsockTcpStreamSetSockOptFn WinsockTcpStream_setsockopt = CallSetSockOpt; +WinsockTcpStreamGetSockOptFn WinsockTcpStream_getsockopt = CallGetSockOpt; +WinsockTcpStreamCloseSocketFn WinsockTcpStream_closesocket = CallCloseSocket; +WinsockTcpStreamIoctlSocketFn WinsockTcpStream_ioctlsocket = CallIoctlSocket; +WinsockTcpStreamSelectFn WinsockTcpStream_select = CallSelect; +WinsockTcpStreamWSAGetLastErrorFn WinsockTcpStream_WSAGetLastError = CallWSAGetLastError; static SOCKET WSAAPI CallSocket(int af, int type, int protocol) { @@ -87,7 +87,6 @@ static int WSAAPI CallWSAGetLastError(void) enum { - SEND_TIMEOUT_MILLISECONDS = 5000, /* Caps the time a single connect() attempt can stall the service thread. Windows' default connect() to a refused loopback port retries internally for ~2 s before returning WSAECONNREFUSED, which throttles the service @@ -113,15 +112,16 @@ static SolidSyslogSsize Read(struct SolidSyslogStream* self, void* buffer, size_ static void Close(struct SolidSyslogStream* self); static SOCKET OpenAndConfigureSocket(void); +static bool ConfigureSocket(SOCKET fd); static void EnableTcpNoDelay(SOCKET fd); -static void SetSendTimeout(SOCKET fd); static inline bool IsSocketValid(SOCKET fd); static bool ConnectOrCloseOnFailure(struct SolidSyslogWinsockTcpStream* stream, const struct sockaddr_in* sin); static bool Connect(SOCKET fd, const struct sockaddr_in* sin); -static bool SetNonBlocking(SOCKET fd, u_long nonblocking); +static bool SetNonBlocking(SOCKET fd); static bool WaitForConnectCompletion(SOCKET fd); static bool ReadDeferredConnectError(SOCKET fd); static bool WroteAllBytes(int sent, size_t expected); +static inline bool WouldBlock(int wsaError); SOLIDSYSLOG_STATIC_ASSERT(sizeof(struct SolidSyslogWinsockTcpStream) <= SOLIDSYSLOG_WINSOCK_TCP_STREAM_SIZE, "SOLIDSYSLOG_WINSOCK_TCP_STREAM_SIZE is too small for struct SolidSyslogWinsockTcpStream"); @@ -164,33 +164,30 @@ static bool Open(struct SolidSyslogStream* self, const struct SolidSyslogAddress return connected; } +/* Non-blocking from the start: connect() returns WSAEWOULDBLOCK and the wait + * is bounded via select(); Send/Read never block the service thread on a + * wedged peer or a full kernel send buffer. */ static SOCKET OpenAndConfigureSocket(void) { SOCKET fd = WinsockTcpStream_socket(AF_INET, SOCK_STREAM, 0); - if (IsSocketValid(fd)) + if (IsSocketValid(fd) && !ConfigureSocket(fd)) { - EnableTcpNoDelay(fd); - SetSendTimeout(fd); + WinsockTcpStream_closesocket(fd); + fd = INVALID_SOCKET; } return fd; } -static void EnableTcpNoDelay(SOCKET fd) +static bool ConfigureSocket(SOCKET fd) { - int enable = 1; - WinsockTcpStream_setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (const char*) &enable, (int) sizeof(enable)); + EnableTcpNoDelay(fd); + return SetNonBlocking(fd); } -/* Caps blocking time of send() so a wedged peer can't make a single - * SolidSyslog_Service() call hang. On expiry, send() returns SOCKET_ERROR - * (last error WSAETIMEDOUT), the Send vtable returns false, and the Service - * loop closes and reconnects on the next attempt; store-and-forward replays - * the message on the fresh socket. Hard-coded for now; port-time CMake - * override is a future option. */ -static void SetSendTimeout(SOCKET fd) +static void EnableTcpNoDelay(SOCKET fd) { - DWORD timeout = SEND_TIMEOUT_MILLISECONDS; - WinsockTcpStream_setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, (const char*) &timeout, (int) sizeof(timeout)); + int enable = 1; + WinsockTcpStream_setsockopt(fd, IPPROTO_TCP, TCP_NODELAY, (const char*) &enable, (int) sizeof(enable)); } static inline bool IsSocketValid(SOCKET fd) @@ -210,43 +207,31 @@ static bool ConnectOrCloseOnFailure(struct SolidSyslogWinsockTcpStream* stream, } /* Non-blocking connect with bounded wait. Windows' default blocking connect() - to a refused loopback port retries internally for ~2 s before returning - WSAECONNREFUSED — slow enough that the BlockStore service thread's drain - rate during an outage is throttled to ~0.5 records/s, preventing the - discard policy from firing in BDD outage scenarios. The non-blocking path - bounds each connect attempt to CONNECT_TIMEOUT_MILLISECONDS; the next - service-thread iteration retries. Blocking mode is restored before return - on success so subsequent send()/recv() honour SO_SNDTIMEO. */ + * to a refused loopback port retries internally for ~2 s before returning + * WSAECONNREFUSED — slow enough that the BlockStore service thread's drain + * rate during an outage is throttled to ~0.5 records/s, preventing the + * discard policy from firing in BDD outage scenarios. The non-blocking path + * bounds each connect attempt to CONNECT_TIMEOUT_MILLISECONDS; the socket + * stays non-blocking thereafter so Send/Read are also fail-fast. */ static bool Connect(SOCKET fd, const struct sockaddr_in* sin) { - bool ready = SetNonBlocking(fd, 1); bool connected = false; + int rc = WinsockTcpStream_connect(fd, (const struct sockaddr*) sin, (int) sizeof(*sin)); - if (ready) + if (rc != SOCKET_ERROR) { - int rc = WinsockTcpStream_connect(fd, (const struct sockaddr*) sin, (int) sizeof(*sin)); - - if (rc != SOCKET_ERROR) - { - connected = true; - } - else if (WinsockTcpStream_WSAGetLastError() == WSAEWOULDBLOCK) - { - connected = WaitForConnectCompletion(fd) && ReadDeferredConnectError(fd); - } + connected = true; } - - if (connected) + else if (WinsockTcpStream_WSAGetLastError() == WSAEWOULDBLOCK) { - connected = SetNonBlocking(fd, 0); + connected = WaitForConnectCompletion(fd) && ReadDeferredConnectError(fd); } - return connected; } -static bool SetNonBlocking(SOCKET fd, u_long nonblocking) +static bool SetNonBlocking(SOCKET fd) { - u_long mode = nonblocking; + u_long mode = 1; return WinsockTcpStream_ioctlsocket(fd, (long) FIONBIO, &mode) != SOCKET_ERROR; } @@ -260,10 +245,8 @@ static bool WaitForConnectCompletion(SOCKET fd) FD_ZERO(&errorSet); FD_SET(fd, &errorSet); - struct timeval timeout = { - .tv_sec = CONNECT_TIMEOUT_MILLISECONDS / MILLISECONDS_PER_SECOND, - .tv_usec = (CONNECT_TIMEOUT_MILLISECONDS % MILLISECONDS_PER_SECOND) * MICROSECONDS_PER_MILLISECOND - }; + struct timeval timeout = {.tv_sec = CONNECT_TIMEOUT_MILLISECONDS / MILLISECONDS_PER_SECOND, + .tv_usec = (CONNECT_TIMEOUT_MILLISECONDS % MILLISECONDS_PER_SECOND) * MICROSECONDS_PER_MILLISECOND}; /* nfds is ignored on Winsock (Windows uses fd_set as a literal array, not a bitmask), but POSIX-portable callers must pass the highest fd + 1. @@ -286,14 +269,17 @@ static bool Send(struct SolidSyslogStream* self, const void* buffer, size_t size { struct SolidSyslogWinsockTcpStream* stream = (struct SolidSyslogWinsockTcpStream*) self; int sent = WinsockTcpStream_send(stream->fd, (const char*) buffer, (int) size, 0); - return WroteAllBytes(sent, size); + bool ok = WroteAllBytes(sent, size); + + if (!ok) + { + Close(self); + } + return ok; } -/* Winsock send() does not have signal-interruption semantics, so the EINTR - * retry loop present in PosixTcpStream is not needed here. Any failure - * (including WSAETIMEDOUT from SO_SNDTIMEO firing) and any short return - * propagate via WroteAllBytes; the caller closes and reconnects, store- - * and-forward replays the message on the fresh socket. */ +/* Non-blocking single-call contract: short write or any error means the + * connection is gone; the caller closes and reconnects on the next attempt. */ static bool WroteAllBytes(int sent, size_t expected) { return (sent >= 0) && ((size_t) sent == expected); @@ -302,7 +288,27 @@ static bool WroteAllBytes(int sent, size_t expected) static SolidSyslogSsize Read(struct SolidSyslogStream* self, void* buffer, size_t size) { struct SolidSyslogWinsockTcpStream* stream = (struct SolidSyslogWinsockTcpStream*) self; - return (SolidSyslogSsize) WinsockTcpStream_recv(stream->fd, (char*) buffer, (int) size, 0); + int n = WinsockTcpStream_recv(stream->fd, (char*) buffer, (int) size, 0); + SolidSyslogSsize result = -1; + + if (n > 0) + { + result = (SolidSyslogSsize) n; + } + else if ((n == SOCKET_ERROR) && WouldBlock(WinsockTcpStream_WSAGetLastError())) + { + result = 0; + } + else + { + Close(self); + } + return result; +} + +static inline bool WouldBlock(int wsaError) +{ + return wsaError == WSAEWOULDBLOCK; } static void Close(struct SolidSyslogStream* self) diff --git a/Tests/SolidSyslogWinsockTcpStreamTest.cpp b/Tests/SolidSyslogWinsockTcpStreamTest.cpp index 560a8a5d..ce80d426 100644 --- a/Tests/SolidSyslogWinsockTcpStreamTest.cpp +++ b/Tests/SolidSyslogWinsockTcpStreamTest.cpp @@ -94,12 +94,6 @@ TEST(SolidSyslogWinsockTcpStream, OpenEnablesTcpNoDelay) CHECK_TRUE(WinsockFake_HasSetSockOpt(IPPROTO_TCP, TCP_NODELAY)); } -TEST(SolidSyslogWinsockTcpStream, OpenSetsSendTimeout) -{ - SolidSyslogStream_Open(stream, addr); - CHECK_TRUE(WinsockFake_HasSetSockOpt(SOL_SOCKET, SO_SNDTIMEO)); -} - TEST(SolidSyslogWinsockTcpStream, OpenCallsConnectWithSocketFd) { SolidSyslogStream_Open(stream, addr); @@ -153,22 +147,15 @@ TEST(SolidSyslogWinsockTcpStream, OpenClosesSocketOnConnectFailure) * by Windows' default ~2 s connect()-retry on a refused loopback port. * -------------------------------------------------------------------- */ -TEST(SolidSyslogWinsockTcpStream, OpenSetsNonBlockingModeBeforeConnect) +TEST(SolidSyslogWinsockTcpStream, OpenSetsNonBlockingMode) { SolidSyslogStream_Open(stream, addr); - /* First FIONBIO call is non-blocking on (1) before the connect attempt. */ + /* Single FIONBIO call: non-blocking on (1). The socket stays non-blocking + so Send/Read are also fail-fast — no SO_SNDTIMEO needed. */ + LONGS_EQUAL(1, WinsockFake_FionbioCallCount()); LONGS_EQUAL(1, WinsockFake_FionbioArgAt(0)); } -TEST(SolidSyslogWinsockTcpStream, OpenRestoresBlockingModeAfterSuccessfulConnect) -{ - SolidSyslogStream_Open(stream, addr); - /* Two FIONBIO calls total: non-blocking on (1) before connect, blocking - back on (0) after success — keeps SO_SNDTIMEO honoured for send(). */ - LONGS_EQUAL(2, WinsockFake_FionbioCallCount()); - LONGS_EQUAL(0, WinsockFake_FionbioArgAt(1)); -} - TEST(SolidSyslogWinsockTcpStream, OpenSkipsSelectWhenConnectReturnsImmediately) { /* Default fake connect returns 0 (immediate success); select must not be @@ -327,6 +314,16 @@ TEST(SolidSyslogWinsockTcpStream, SendDoesNotRetryAfterShortWrite) LONGS_EQUAL(1, WinsockFake_SendCallCount()); } +TEST(SolidSyslogWinsockTcpStream, SendClosesSocketOnFailure) +{ + SolidSyslogStream_Open(stream, addr); + SOCKET openFd = WinsockFake_SocketFd(); + WinsockFake_SetSendFails(true); + SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN); + LONGS_EQUAL(1, WinsockFake_CloseCallCount()); + CHECK(openFd == WinsockFake_LastClosedFd()); +} + TEST(SolidSyslogWinsockTcpStream, CloseCallsCloseOnce) { SolidSyslogStream_Open(stream, addr); @@ -396,6 +393,39 @@ TEST(SolidSyslogWinsockTcpStream, ReadReturnsRecvReturnValue) LONGS_EQUAL(7, n); } +TEST(SolidSyslogWinsockTcpStream, ReadReturnsZeroOnWouldBlock) +{ + SolidSyslogStream_Open(stream, addr); + WinsockFake_FailNextRecvWithLastError(WSAEWOULDBLOCK); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(0, n); +} + +TEST(SolidSyslogWinsockTcpStream, ReadReturnsNegativeOneOnEofAndClosesSocket) +{ + SolidSyslogStream_Open(stream, addr); + SOCKET openFd = WinsockFake_SocketFd(); + WinsockFake_SetRecvReturn(0); /* EOF */ + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(-1, n); + LONGS_EQUAL(1, WinsockFake_CloseCallCount()); + CHECK(openFd == WinsockFake_LastClosedFd()); +} + +TEST(SolidSyslogWinsockTcpStream, ReadReturnsNegativeOneOnErrorAndClosesSocket) +{ + SolidSyslogStream_Open(stream, addr); + SOCKET openFd = WinsockFake_SocketFd(); + WinsockFake_FailNextRecvWithLastError(WSAECONNRESET); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(-1, n); + LONGS_EQUAL(1, WinsockFake_CloseCallCount()); + CHECK(openFd == WinsockFake_LastClosedFd()); +} + TEST(SolidSyslogWinsockTcpStream, DestroyClosesOpenSocket) { SolidSyslogStream_Open(stream, addr); diff --git a/Tests/Support/WinsockFake.c b/Tests/Support/WinsockFake.c index f40684a1..f86f205c 100644 --- a/Tests/Support/WinsockFake.c +++ b/Tests/Support/WinsockFake.c @@ -56,6 +56,8 @@ static SOCKET lastRecvFd; static const void* lastRecvBuf; static size_t lastRecvLen; static int lastRecvFlags; +static bool nextRecvShouldFailWithLastError; +static int nextRecvLastError; static bool connectFailWithLastError; static int connectLastErrorOnFail; @@ -138,12 +140,14 @@ void WinsockFake_Reset(void) } lastSendFd = INVALID_SOCKET; - recvCallCount = 0; - recvReturn = 0; - lastRecvFd = INVALID_SOCKET; - lastRecvBuf = NULL; - lastRecvLen = 0; - lastRecvFlags = 0; + recvCallCount = 0; + recvReturn = 0; + lastRecvFd = INVALID_SOCKET; + lastRecvBuf = NULL; + lastRecvLen = 0; + lastRecvFlags = 0; + nextRecvShouldFailWithLastError = false; + nextRecvLastError = 0; connectFailWithLastError = false; connectLastErrorOnFail = 0; @@ -496,6 +500,12 @@ void WinsockFake_SetRecvReturn(int value) recvReturn = value; } +void WinsockFake_FailNextRecvWithLastError(int wsaError) +{ + nextRecvShouldFailWithLastError = true; + nextRecvLastError = wsaError; +} + /* recv accessors */ int WinsockFake_RecvCallCount(void) @@ -693,6 +703,12 @@ int WSAAPI WinsockFake_recv(SOCKET s, char* buf, int len, int flags) lastRecvBuf = buf; lastRecvLen = (size_t) len; lastRecvFlags = flags; + if (nextRecvShouldFailWithLastError) + { + WSASetLastError(nextRecvLastError); + nextRecvShouldFailWithLastError = false; + return SOCKET_ERROR; + } return recvReturn; } diff --git a/Tests/Support/WinsockFake.h b/Tests/Support/WinsockFake.h index 5f2b1530..90d2ff51 100644 --- a/Tests/Support/WinsockFake.h +++ b/Tests/Support/WinsockFake.h @@ -60,6 +60,9 @@ EXTERN_C_BEGIN /* recv configuration */ void WinsockFake_SetRecvReturn(int value); + /* When set, the next recv returns SOCKET_ERROR with WSAGetLastError == wsaError + (e.g. WSAEWOULDBLOCK for the non-blocking would-block path). One-shot. */ + void WinsockFake_FailNextRecvWithLastError(int wsaError); /* recv accessors */ int WinsockFake_RecvCallCount(void); From de10a150b0ddccd1c0e6b633e21ac3881f5b86a1 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 06:55:43 +0100 Subject: [PATCH 04/16] feat: S12.14 TlsStream non-blocking + fail-fast MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five coordinated changes so TLS works on top of the now-non-blocking PosixTcp/WinsockTcp streams: - BIO read translates would-block (0) into BIO_set_retry_read + return -1, and clears retry on EOF/error. Without this, OpenSSL would treat the transport's would-block as EOF and abort the handshake on first poll. - BIO write clears retry flags on transport-Send failure so SSL_write surfaces SSL_ERROR_SYSCALL rather than spinning on a closed transport. - PerformHandshake drives SSL_connect in a bounded retry loop with a 5 s budget and a 1 ms poll interval — sleeps on WANT_READ / WANT_WRITE, fails fast on any hard SSL error. - Send is single-call, fail-fast: SSL_write != size closes the SSL session and the underlying transport so the StreamSender's reconnect path picks up on the next service tick. - Read returns 0 on WANT_READ (mirrors transport contract); anything else (errors, peer close, mid-stream renegotiation needing WANT_WRITE) takes the close + -1 path. Comment in TlsStream_Read records the handshake- vs-steady-state distinction explicitly. - TlsStream_Close is now idempotent so internal-close-from-Send/Read followed by an explicit Close from the StreamSender doesn't double-free. OpenSslFake gains SSL_connect return-sequence injection, SSL_get_error, SSL_write/read return overrides, and BIO_set_flags / BIO_clear_flags spies so the new code paths are covered without touching the real OpenSSL during unit tests. TlsStream_sleep is a function-pointer seam (Windows Sleep / POSIX nanosleep by default; tests UT_PTR_SET to a no-op). Coverage 100%. Validated locally on MSVC: 973 tests pass. --- .../OpenSsl/Source/SolidSyslogTlsStream.c | 160 +++++++++++- .../Source/SolidSyslogTlsStreamInternal.h | 17 ++ Tests/CMakeLists.txt | 1 + Tests/SolidSyslogTlsStreamTest.cpp | 229 ++++++++++++++++++ Tests/StreamFake.c | 8 +- Tests/StreamFake.h | 1 + Tests/Support/OpenSslFake.c | 151 ++++++++++-- Tests/Support/OpenSslFake.h | 17 ++ 8 files changed, 557 insertions(+), 27 deletions(-) create mode 100644 Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h diff --git a/Platform/OpenSsl/Source/SolidSyslogTlsStream.c b/Platform/OpenSsl/Source/SolidSyslogTlsStream.c index 956ec545..7f841f2c 100644 --- a/Platform/OpenSsl/Source/SolidSyslogTlsStream.c +++ b/Platform/OpenSsl/Source/SolidSyslogTlsStream.c @@ -1,14 +1,45 @@ -#include #include #include +#include #include #include #include +#ifdef _WIN32 +#include +#else +#include +#endif + #include "SolidSyslogMacros.h" +#include "SolidSyslogStream.h" #include "SolidSyslogStreamDefinition.h" #include "SolidSyslogTlsStream.h" -#include "SolidSyslogStream.h" +#include "SolidSyslogTlsStreamInternal.h" + +enum +{ + /* Bounded retry budget for the TLS handshake under non-blocking transport. + Real-world handshakes take ~100–500 ms (1–3 RTTs to a cloud SIEM); 5 s + comfortably covers WAN deployments without burning the service thread + indefinitely on a wedged peer. */ + HANDSHAKE_TIMEOUT_MILLISECONDS = 5000, + HANDSHAKE_POLL_INTERVAL_MILLISECONDS = 1 +}; + +static void DefaultSleep(int milliseconds); + +TlsStreamSleepFn TlsStream_sleep = DefaultSleep; + +static void DefaultSleep(int milliseconds) +{ +#ifdef _WIN32 + Sleep((DWORD) milliseconds); +#else + struct timespec ts = {.tv_sec = milliseconds / 1000, .tv_nsec = (long) (milliseconds % 1000) * 1000000L}; + (void) nanosleep(&ts, NULL); +#endif +} struct SolidSyslogAddress; @@ -233,16 +264,49 @@ static inline int TlsStream_TransportBioCreate(BIO* bio) return 1; } +/* Translate the non-blocking transport's Read contract into the OpenSSL BIO + * contract: + * transport > 0 → bytes available, BIO returns the same positive count. + * transport = 0 → would-block. BIO must signal retry via BIO_set_retry_read + * and return -1; without this, OpenSSL treats the 0 as EOF + * and aborts the handshake on the first poll. + * transport < 0 → EOF or error. BIO returns -1 with retry flags cleared so + * OpenSSL surfaces the failure rather than spinning. */ static inline int TlsStream_TransportBioRead(BIO* bio, char* buffer, int size) { struct SolidSyslogStream* transport = (struct SolidSyslogStream*) BIO_get_data(bio); - return (int) SolidSyslogStream_Read(transport, buffer, (size_t) size); + SolidSyslogSsize n = SolidSyslogStream_Read(transport, buffer, (size_t) size); + int result = -1; + + if (n > 0) + { + result = (int) n; + } + else if (n == 0) + { + BIO_set_retry_read(bio); + } + else + { + BIO_clear_retry_flags(bio); + } + return result; } static inline int TlsStream_TransportBioWrite(BIO* bio, const char* buffer, int size) { struct SolidSyslogStream* transport = (struct SolidSyslogStream*) BIO_get_data(bio); - return SolidSyslogStream_Send(transport, buffer, (size_t) size) ? size : -1; + int result = -1; + + if (SolidSyslogStream_Send(transport, buffer, (size_t) size)) + { + result = size; + } + else + { + BIO_clear_retry_flags(bio); + } + return result; } /* Minimal ctrl handler. OpenSSL calls this for a variety of control commands @@ -279,27 +343,105 @@ static inline bool TlsStream_ConfigureExpectedHostname(struct SolidSyslogTlsStre return ok; } +static inline bool IsRetryableSslError(int err) +{ + return (err == SSL_ERROR_WANT_READ) || (err == SSL_ERROR_WANT_WRITE); +} + +static inline bool IsHandshakeBudgetExhausted(int totalSleptMs) +{ + return totalSleptMs >= HANDSHAKE_TIMEOUT_MILLISECONDS; +} + +/* Drive SSL_connect to completion under non-blocking transport. Each call may + * return WANT_READ/WANT_WRITE while waiting for the multi-RTT handshake to + * progress; we sleep briefly between attempts (avoiding a busy spin) until + * either the handshake completes, hits a hard error, or the bounded budget + * expires. */ static inline bool TlsStream_PerformHandshake(struct SolidSyslogTlsStream* stream) { - return SSL_connect(stream->ssl) > 0; + int totalSleptMs = 0; + bool result = false; + bool done = false; + + while (!done) + { + int rc = SSL_connect(stream->ssl); + if (rc > 0) + { + result = true; + done = true; + } + else + { + int err = SSL_get_error(stream->ssl, rc); + if (!IsRetryableSslError(err) || IsHandshakeBudgetExhausted(totalSleptMs)) + { + done = true; + } + else + { + TlsStream_sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS); + totalSleptMs += HANDSHAKE_POLL_INTERVAL_MILLISECONDS; + } + } + } + return result; } static inline bool TlsStream_Send(struct SolidSyslogStream* self, const void* buffer, size_t size) { struct SolidSyslogTlsStream* stream = (struct SolidSyslogTlsStream*) self; - return SSL_write(stream->ssl, buffer, (int) size) > 0; + int rc = SSL_write(stream->ssl, buffer, (int) size); + bool ok = (rc > 0) && ((size_t) rc == size); + + if (!ok) + { + TlsStream_Close(self); + } + return ok; } +/* SSL_read has two distinct modes worth keeping straight: + * 1. Steady-state application read: bytes available → return them; nothing + * to read right now → SSL_ERROR_WANT_READ → return 0 mirrors the transport + * Read contract. + * 2. Renegotiation or alerts mid-stream: SSL_read may need to write (server + * requested re-key), surfacing as SSL_ERROR_WANT_WRITE. Under fail-fast + * semantics this is a transport failure — close internally; the caller + * reopens, store-and-forward replays. Same rule for any other SSL error. + * Anything below the WANT_READ branch therefore takes the Close path. */ static inline SolidSyslogSsize TlsStream_Read(struct SolidSyslogStream* self, void* buffer, size_t size) { struct SolidSyslogTlsStream* stream = (struct SolidSyslogTlsStream*) self; - return (SolidSyslogSsize) SSL_read(stream->ssl, buffer, (int) size); + int rc = SSL_read(stream->ssl, buffer, (int) size); + SolidSyslogSsize result = -1; + + if (rc > 0) + { + result = (SolidSyslogSsize) rc; + } + else if (SSL_get_error(stream->ssl, rc) == SSL_ERROR_WANT_READ) + { + result = 0; + } + else + { + TlsStream_Close(self); + } + return result; } +/* Idempotent: Send/Read may close internally on failure, after which the + * StreamSender's reconnect path or the caller's Destroy may call Close + * again. Skipping when ssl is already NULL keeps that safe. */ static inline void TlsStream_Close(struct SolidSyslogStream* self) { struct SolidSyslogTlsStream* stream = (struct SolidSyslogTlsStream*) self; - SSL_shutdown(stream->ssl); - TlsStream_ReleaseHandshakeState(stream); + if (stream->ssl != NULL) + { + SSL_shutdown(stream->ssl); + TlsStream_ReleaseHandshakeState(stream); + } SolidSyslogStream_Close(stream->config.transport); } diff --git a/Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h b/Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h new file mode 100644 index 00000000..6b9ac92e --- /dev/null +++ b/Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h @@ -0,0 +1,17 @@ +#ifndef SOLIDSYSLOGTLSSTREAMINTERNAL_H +#define SOLIDSYSLOGTLSSTREAMINTERNAL_H + +#include "ExternC.h" + +EXTERN_C_BEGIN + + /* Function-pointer seam for the TLS handshake retry loop. Production wires + this to a platform-conditional sleep (Sleep on Windows, nanosleep on + POSIX); tests UT_PTR_SET it to a no-op so the bounded retry budget + drains in microseconds rather than its real wall-clock duration. */ + typedef void (*TlsStreamSleepFn)(int milliseconds); + extern TlsStreamSleepFn TlsStream_sleep; + +EXTERN_C_END + +#endif /* SOLIDSYSLOGTLSSTREAMINTERNAL_H */ diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt index 54fd594a..55f708db 100644 --- a/Tests/CMakeLists.txt +++ b/Tests/CMakeLists.txt @@ -141,6 +141,7 @@ endif() if(SOLIDSYSLOG_OPENSSL) target_link_libraries(${PROJECT_NAME}Tests PRIVATE OpenSslFakes) + target_include_directories(${PROJECT_NAME}Tests PRIVATE ${CMAKE_SOURCE_DIR}/Platform/OpenSsl/Source) endif() if(HAVE_WINDOWS_PLATFORM OR SOLIDSYSLOG_WINSOCK) diff --git a/Tests/SolidSyslogTlsStreamTest.cpp b/Tests/SolidSyslogTlsStreamTest.cpp index f5c663fb..1baefc9d 100644 --- a/Tests/SolidSyslogTlsStreamTest.cpp +++ b/Tests/SolidSyslogTlsStreamTest.cpp @@ -8,10 +8,20 @@ #include "SolidSyslogAddress.h" #include "SolidSyslogStream.h" #include "SolidSyslogTlsStream.h" +#include "SolidSyslogTlsStreamInternal.h" #include "SolidSyslogTransport.h" #include "StreamFake.h" #include "CppUTest/TestHarness.h" +static int g_sleepCallCount; +static int g_lastSleepMs; + +static void NoOpSleep(int milliseconds) +{ + g_sleepCallCount++; + g_lastSleepMs = milliseconds; +} + // clang-format off TEST_GROUP(SolidSyslogTlsStream) { @@ -25,6 +35,9 @@ TEST_GROUP(SolidSyslogTlsStream) void setup() override { OpenSslFake_Reset(); + g_sleepCallCount = 0; + g_lastSleepMs = 0; + UT_PTR_SET(TlsStream_sleep, NoOpSleep); transport = StreamFake_Create(); config.transport = transport; // cppcheck-suppress unreadVariable -- used across TEST_GROUP methods; cppcheck does not model CppUTest macros @@ -831,3 +844,219 @@ TEST(SolidSyslogTlsStream, DefaultPortMatchesRfc5425) { LONGS_EQUAL(6514, SOLIDSYSLOG_TLS_DEFAULT_PORT); } + +/* ------------------------------------------------------------------------- + * Default platform sleep — exercised without the no-op override so the + * production sleep helper is reached at least once for coverage. Calls with + * 0 ms keep the test fast. + * ------------------------------------------------------------------------- */ + +// clang-format off +TEST_GROUP(SolidSyslogTlsStreamDefaultSleep) +{ +}; +// clang-format on + +TEST(SolidSyslogTlsStreamDefaultSleep, DefaultSleepReturnsImmediatelyForZero) +{ + TlsStream_sleep(0); +} + +/* ------------------------------------------------------------------------- + * Non-blocking BIO read translation. Under the new transport contract a + * 0 return means "would-block, retry"; without BIO_set_retry_read OpenSSL + * would treat that as EOF and abort the handshake on the first poll. + * ------------------------------------------------------------------------- */ + +TEST(SolidSyslogTlsStream, BioReadCallbackSignalsRetryWhenTransportWouldBlock) +{ + SolidSyslogStream_Open(stream, addr); + StreamFake_SetReadReturn(transport, 0); + int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); + char buf[16]; + int rc = readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); + LONGS_EQUAL(-1, rc); + LONGS_EQUAL(1, OpenSslFake_BioSetFlagsCallCount()); +} + +TEST(SolidSyslogTlsStream, BioReadCallbackClearsRetryOnHardError) +{ + SolidSyslogStream_Open(stream, addr); + StreamFake_SetReadReturn(transport, -1); + int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); + char buf[16]; + int rc = readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); + LONGS_EQUAL(-1, rc); + LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()); +} + +TEST(SolidSyslogTlsStream, BioReadCallbackReturnsBytesWhenTransportHasData) +{ + SolidSyslogStream_Open(stream, addr); + StreamFake_SetReadReturn(transport, 7); + int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); + char buf[16]; + int rc = readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); + LONGS_EQUAL(7, rc); + /* No retry signal needed: positive return is the success path. */ + LONGS_EQUAL(0, OpenSslFake_BioSetFlagsCallCount()); +} + +TEST(SolidSyslogTlsStream, BioWriteCallbackClearsRetryOnTransportFailure) +{ + /* When the transport's fail-fast Send returns false the BIO must clear + any stale retry flag and return -1 so OpenSSL surfaces SSL_ERROR_SYSCALL + rather than spinning on a closed transport. */ + SolidSyslogStream_Open(stream, addr); + StreamFake_SetSendFails(transport, true); + int (*writeFn)(BIO*, const char*, int) = OpenSslFake_LastBioWriteCallback(); + const char msg[] = "hi"; + int rc = writeFn(OpenSslFake_LastBioReturned(), msg, (int) sizeof(msg)); + LONGS_EQUAL(-1, rc); + LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()); +} + +/* ------------------------------------------------------------------------- + * Bounded handshake retry loop. SSL_connect under non-blocking transport + * will emit WANT_READ/WANT_WRITE between RTTs; the loop must drive it to + * completion within HANDSHAKE_TIMEOUT_MILLISECONDS. + * ------------------------------------------------------------------------- */ + +TEST(SolidSyslogTlsStream, OpenRetriesHandshakeOnWantRead) +{ + /* First call: WANT_READ; second call: success. */ + int seq[] = {-1, 1}; + OpenSslFake_SetConnectReturnSequence(seq, 2); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + CHECK_TRUE(SolidSyslogStream_Open(stream, addr)); + LONGS_EQUAL(2, OpenSslFake_ConnectCallCount()); +} + +TEST(SolidSyslogTlsStream, OpenSleepsBetweenHandshakeRetries) +{ + int seq[] = {-1, 1}; + OpenSslFake_SetConnectReturnSequence(seq, 2); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + SolidSyslogStream_Open(stream, addr); + LONGS_EQUAL(1, g_sleepCallCount); +} + +TEST(SolidSyslogTlsStream, OpenRetriesHandshakeOnWantWrite) +{ + /* WANT_WRITE arises when SSL needs to send (e.g. during the handshake + finished message under non-blocking transport with a temporarily-full + send buffer). Same retry treatment as WANT_READ. */ + int seq[] = {-1, 1}; + OpenSslFake_SetConnectReturnSequence(seq, 2); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_WRITE); + CHECK_TRUE(SolidSyslogStream_Open(stream, addr)); + LONGS_EQUAL(2, OpenSslFake_ConnectCallCount()); +} + +TEST(SolidSyslogTlsStream, OpenFailsWhenHandshakeNeverCompletes) +{ + /* SSL_connect always returns -1 with WANT_READ — handshake never makes + progress, so the bounded budget should expire and Open returns false. */ + int seq[] = {-1}; + OpenSslFake_SetConnectReturnSequence(seq, 1); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); +} + +TEST(SolidSyslogTlsStream, OpenFailsImmediatelyOnHardSslError) +{ + /* Non-WANT error (e.g. SSL_ERROR_SSL) is fail-fast — no retry budget burn. */ + int seq[] = {-1}; + OpenSslFake_SetConnectReturnSequence(seq, 1); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_SSL); + CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); + LONGS_EQUAL(1, OpenSslFake_ConnectCallCount()); + LONGS_EQUAL(0, g_sleepCallCount); +} + +/* ------------------------------------------------------------------------- + * Send fail-fast: any non-success closes the SSL session and the underlying + * transport so the StreamSender's reconnect path runs on the next tick. + * ------------------------------------------------------------------------- */ + +TEST(SolidSyslogTlsStream, SendClosesSslOnWriteFailure) +{ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetWriteFails(true); + const char msg[] = "hi"; + SolidSyslogStream_Send(stream, msg, sizeof(msg)); + LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); + LONGS_EQUAL(1, OpenSslFake_FreeCallCount()); +} + +TEST(SolidSyslogTlsStream, SendClosesTransportOnWriteFailure) +{ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetWriteFails(true); + const char msg[] = "hi"; + SolidSyslogStream_Send(stream, msg, sizeof(msg)); + LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)); +} + +TEST(SolidSyslogTlsStream, SendReturnsFalseOnShortWrite) +{ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetWriteReturn(3); + const char msg[] = "hello"; + CHECK_FALSE(SolidSyslogStream_Send(stream, msg, sizeof(msg))); +} + +/* ------------------------------------------------------------------------- + * Read non-blocking contract. + * ------------------------------------------------------------------------- */ + +TEST(SolidSyslogTlsStream, ReadReturnsZeroOnWantRead) +{ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetReadReturn(-1); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(0, n); +} + +TEST(SolidSyslogTlsStream, ReadReturnsNegativeOneOnHardErrorAndClosesSsl) +{ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetReadReturn(-1); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_SSL); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(-1, n); + LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); + LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)); +} + +TEST(SolidSyslogTlsStream, ReadReturnsNegativeOneOnZeroReturnAndClosesSsl) +{ + /* SSL_read returns 0 → SSL_ERROR_ZERO_RETURN (clean shutdown by peer). */ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetReadReturn(0); + OpenSslFake_SetGetErrorReturn(SSL_ERROR_ZERO_RETURN); + char buf[16]; + SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); + LONGS_EQUAL(-1, n); + LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); +} + +/* ------------------------------------------------------------------------- + * Close idempotency. Send/Read may close internally on failure; subsequent + * Close from the StreamSender's reconnect path or Destroy must not crash + * or double-free. + * ------------------------------------------------------------------------- */ + +TEST(SolidSyslogTlsStream, CloseAfterInternalCloseFromSendFailureDoesNotDoubleFree) +{ + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetWriteFails(true); + const char msg[] = "hi"; + SolidSyslogStream_Send(stream, msg, sizeof(msg)); /* internal close */ + SolidSyslogStream_Close(stream); /* second close — must be safe */ + LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); + LONGS_EQUAL(1, OpenSslFake_FreeCallCount()); +} diff --git a/Tests/StreamFake.c b/Tests/StreamFake.c index bbedc6f8..de4291c9 100644 --- a/Tests/StreamFake.c +++ b/Tests/StreamFake.c @@ -12,6 +12,7 @@ struct StreamFake int sendCallCount; const void* lastSendBuf; size_t lastSendSize; + bool sendFails; int readCallCount; void* lastReadBuf; size_t lastReadSize; @@ -33,7 +34,7 @@ static bool Send(struct SolidSyslogStream* self, const void* buffer, size_t size fake->sendCallCount++; fake->lastSendBuf = buffer; fake->lastSendSize = size; - return true; + return !fake->sendFails; } static SolidSyslogSsize Read(struct SolidSyslogStream* self, void* buffer, size_t size) @@ -116,6 +117,11 @@ void StreamFake_SetOpenFails(struct SolidSyslogStream* stream, bool fails) ((struct StreamFake*) stream)->openFails = fails; } +void StreamFake_SetSendFails(struct SolidSyslogStream* stream, bool fails) +{ + ((struct StreamFake*) stream)->sendFails = fails; +} + int StreamFake_CloseCallCount(struct SolidSyslogStream* stream) { return ((struct StreamFake*) stream)->closeCallCount; diff --git a/Tests/StreamFake.h b/Tests/StreamFake.h index 9d8ff24d..7003fb86 100644 --- a/Tests/StreamFake.h +++ b/Tests/StreamFake.h @@ -25,6 +25,7 @@ EXTERN_C_BEGIN size_t StreamFake_LastReadSize(struct SolidSyslogStream * stream); void StreamFake_SetReadReturn(struct SolidSyslogStream * stream, SolidSyslogSsize value); void StreamFake_SetOpenFails(struct SolidSyslogStream * stream, bool fails); + void StreamFake_SetSendFails(struct SolidSyslogStream * stream, bool fails); int StreamFake_CloseCallCount(struct SolidSyslogStream * stream); EXTERN_C_END diff --git a/Tests/Support/OpenSslFake.c b/Tests/Support/OpenSslFake.c index a2e1e37f..928aa023 100644 --- a/Tests/Support/OpenSslFake.c +++ b/Tests/Support/OpenSslFake.c @@ -114,6 +114,8 @@ static bool set1HostFails; static int connectCallCount; static SSL* lastConnectSslArg; static bool connectFails; +static int connectReturnSequence[OPENSSLFAKE_MAX_CONNECT_RETURNS]; +static int connectReturnSequenceLen; /* SSL_write */ static int writeCallCount; @@ -121,12 +123,25 @@ static SSL* lastWriteSslArg; static const void* lastWriteBuf; static int lastWriteSize; static bool writeFails; +static bool writeReturnOverride; +static int writeReturnValue; /* SSL_read */ static int sslReadCallCount; static SSL* lastSslReadSslArg; static void* lastSslReadBuf; static int lastSslReadSize; +static bool readReturnOverride; +static int readReturnValue; + +/* SSL_get_error */ +static int getErrorCallCount; +static int getErrorReturnValue; + +/* BIO_set_flags / BIO_clear_flags */ +static int bioSetFlagsCallCount; +static int lastBioSetFlags; +static int bioClearFlagsCallCount; /* SSL_shutdown */ static int shutdownCallCount; @@ -200,31 +215,45 @@ void OpenSslFake_Reset(void) { fakeBios[i].data = NULL; } - lastSetDataBioArg = NULL; - lastSetDataArg = NULL; - lastGetDataBioArg = NULL; - setBioCallCount = 0; - lastSetBioSslArg = NULL; - lastSetBioReadBioArg = NULL; - lastSetBioWriteBioArg = NULL; - lastSslCtrlSslArg = NULL; - lastSniHostname = NULL; - sniHostnameFails = false; - lastSet1HostSslArg = NULL; - lastSet1Host = NULL; - set1HostFails = false; - connectCallCount = 0; - lastConnectSslArg = NULL; - connectFails = false; + lastSetDataBioArg = NULL; + lastSetDataArg = NULL; + lastGetDataBioArg = NULL; + setBioCallCount = 0; + lastSetBioSslArg = NULL; + lastSetBioReadBioArg = NULL; + lastSetBioWriteBioArg = NULL; + lastSslCtrlSslArg = NULL; + lastSniHostname = NULL; + sniHostnameFails = false; + lastSet1HostSslArg = NULL; + lastSet1Host = NULL; + set1HostFails = false; + connectCallCount = 0; + lastConnectSslArg = NULL; + connectFails = false; + connectReturnSequenceLen = 0; + for (int i = 0; i < OPENSSLFAKE_MAX_CONNECT_RETURNS; i++) + { + connectReturnSequence[i] = 0; + } writeCallCount = 0; lastWriteSslArg = NULL; lastWriteBuf = NULL; lastWriteSize = 0; writeFails = false; + writeReturnOverride = false; + writeReturnValue = 0; sslReadCallCount = 0; lastSslReadSslArg = NULL; lastSslReadBuf = NULL; lastSslReadSize = 0; + readReturnOverride = false; + readReturnValue = 0; + getErrorCallCount = 0; + getErrorReturnValue = 0; + bioSetFlagsCallCount = 0; + lastBioSetFlags = 0; + bioClearFlagsCallCount = 0; shutdownCallCount = 0; lastShutdownSslArg = NULL; freeCallCount = 0; @@ -754,9 +783,16 @@ void OpenSslFake_SetSet1HostFails(bool fails) int SSL_connect(SSL* ssl) { + int rc = connectFails ? -1 : 1; + int callIndex = connectCallCount; connectCallCount++; lastConnectSslArg = ssl; - return connectFails ? -1 : 1; + if (connectReturnSequenceLen > 0) + { + int idx = (callIndex < connectReturnSequenceLen) ? callIndex : (connectReturnSequenceLen - 1); + rc = connectReturnSequence[idx]; + } + return rc; } void OpenSslFake_SetConnectFails(bool fails) @@ -764,12 +800,26 @@ void OpenSslFake_SetConnectFails(bool fails) connectFails = fails; } +void OpenSslFake_SetConnectReturnSequence(const int* values, int count) +{ + int safe = (count < OPENSSLFAKE_MAX_CONNECT_RETURNS) ? count : OPENSSLFAKE_MAX_CONNECT_RETURNS; + for (int i = 0; i < safe; i++) + { + connectReturnSequence[i] = values[i]; + } + connectReturnSequenceLen = safe; +} + int SSL_write(SSL* ssl, const void* buf, int num) { writeCallCount++; lastWriteSslArg = ssl; lastWriteBuf = buf; lastWriteSize = num; + if (writeReturnOverride) + { + return writeReturnValue; + } return writeFails ? -1 : num; } @@ -778,15 +828,82 @@ void OpenSslFake_SetWriteFails(bool fails) writeFails = fails; } +void OpenSslFake_SetWriteReturn(int value) +{ + writeReturnOverride = true; + writeReturnValue = value; +} + int SSL_read(SSL* ssl, void* buf, int num) { sslReadCallCount++; lastSslReadSslArg = ssl; lastSslReadBuf = buf; lastSslReadSize = num; + if (readReturnOverride) + { + return readReturnValue; + } return num; } +void OpenSslFake_SetReadReturn(int value) +{ + readReturnOverride = true; + readReturnValue = value; +} + +int SSL_get_error(const SSL* ssl, int ret) +{ + (void) ssl; + (void) ret; + getErrorCallCount++; + return getErrorReturnValue; +} + +void OpenSslFake_SetGetErrorReturn(int err) +{ + getErrorReturnValue = err; +} + +int OpenSslFake_GetErrorCallCount(void) +{ + return getErrorCallCount; +} + +/* BIO_set_flags / BIO_clear_flags are macros in OpenSSL that forward to these + * non-inline functions. Faking the underlying calls lets the production code + * use the standard idioms (BIO_set_retry_read, BIO_clear_retry_flags) while + * tests observe whether retry semantics were signalled. */ +void BIO_set_flags(BIO* bio, int flags) +{ + (void) bio; + bioSetFlagsCallCount++; + lastBioSetFlags = flags; +} + +void BIO_clear_flags(BIO* bio, int flags) +{ + (void) bio; + (void) flags; + bioClearFlagsCallCount++; +} + +int OpenSslFake_BioSetFlagsCallCount(void) +{ + return bioSetFlagsCallCount; +} + +int OpenSslFake_LastBioSetFlags(void) +{ + return lastBioSetFlags; +} + +int OpenSslFake_BioClearFlagsCallCount(void) +{ + return bioClearFlagsCallCount; +} + int SSL_shutdown(SSL* ssl) { shutdownCallCount++; diff --git a/Tests/Support/OpenSslFake.h b/Tests/Support/OpenSslFake.h index dda5939b..cc4d125f 100644 --- a/Tests/Support/OpenSslFake.h +++ b/Tests/Support/OpenSslFake.h @@ -29,6 +29,23 @@ EXTERN_C_BEGIN void OpenSslFake_SetBioNewFails(bool fails); void OpenSslFake_SetCipherListFails(bool fails); + /* SSL return-value injection — drive non-blocking I/O paths */ + enum + { + OPENSSLFAKE_MAX_CONNECT_RETURNS = 8 + }; + + void OpenSslFake_SetConnectReturnSequence(const int* values, int count); + void OpenSslFake_SetWriteReturn(int value); + void OpenSslFake_SetReadReturn(int value); + void OpenSslFake_SetGetErrorReturn(int err); + int OpenSslFake_GetErrorCallCount(void); + + /* BIO retry-flag spies */ + int OpenSslFake_BioSetFlagsCallCount(void); + int OpenSslFake_LastBioSetFlags(void); + int OpenSslFake_BioClearFlagsCallCount(void); + /* SSL_CTX_new */ int OpenSslFake_CtxNewCallCount(void); const struct ssl_method_st* OpenSslFake_LastCtxNewMethodArg(void); From 13b11ce8aba8e36f9243a1a261b7510ad42aec9a Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 07:21:23 +0100 Subject: [PATCH 05/16] docs: update DEVLOG for S12.14 buffer-drain decoupling --- DEVLOG.md | 95 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) diff --git a/DEVLOG.md b/DEVLOG.md index f5ae0593..6c5226c3 100644 --- a/DEVLOG.md +++ b/DEVLOG.md @@ -3314,3 +3314,98 @@ in commit `ebddeb4`: whether to re-enable those rules and accept the resulting cleanup sweep. Tracked in [project_clang_tidy_magic_numbers.md](memory). + +## 2026-05-07 — S12.14: decouple buffer drain from sender state + +### Decisions + +- **Eager-drain ProcessMessages** (slice 1). Replaced one-at-a-time pump + with a loop that drains buffer → store until empty, then attempts one + send from the store. Preserved the existing "send directly when store + doesn't retain" bypass via a unified rule (`!HasUnsent` after Write + → best-effort direct send) so the Threaded example's `--store=null` + path keeps working. Refactored `StoreFake` into a small FIFO with a + write counter; left `BufferFake` single-slot and used the production + `SolidSyslogCircularBuffer` + `NullMutex` in the new burst-drain test. + +- **PosixTcpStream non-blocking + fail-fast** (slice 2). `O_NONBLOCK` + via `fcntl` from socket creation; bounded `select()` connect wait + (200 ms, mirrors the Winsock value); `getsockopt(SO_ERROR)` reads the + deferred connect failure. `Send` is single-call, fail-fast — short + write / `EAGAIN` / any error closes the fd internally. `Read` returns + 0 on `EAGAIN`/`EWOULDBLOCK`, -1 + close on EOF/error. Dropped + `SO_SNDTIMEO` (no-op on non-blocking) and the `EINTR` retry loop. + `SocketFake` gained `fcntl`, `select`, connect/recv `errno`-injection, + and `getsockopt(SO_ERROR)` seams. + +- **WinsockTcpStream Send/Read non-blocking + fail-fast** (slice 3). + Stopped restoring blocking mode after a successful connect; dropped + `SO_SNDTIMEO`. `Send` and `Read` mirror the Posix contract. + `WinsockFake_FailNextRecvWithLastError` added to drive the + would-block / error / EOF Read paths. Validated on MSVC. + +- **TlsStream non-blocking + fail-fast** (slice 4). BIO read translates + the transport's would-block (0) into `BIO_set_retry_read` + return -1 + so OpenSSL retries instead of treating it as EOF; BIO write clears + retry on transport-Send failure. `PerformHandshake` drives + `SSL_connect` in a bounded retry loop with a 5 s budget and 1 ms poll + interval — sleeps on `WANT_READ`/`WANT_WRITE`, fails fast on hard + SSL errors. `Send` and `Read` follow the same rule as TCP, with + `Read`'s comment explicitly distinguishing handshake-vs-steady-state + WANT semantics. `TlsStream_Close` is idempotent. `TlsStream_sleep` + is a function-pointer seam (Windows `Sleep` / POSIX `nanosleep` by + default, UT_PTR_SET to no-op in tests). `OpenSslFake` gained + `SSL_connect` return-sequence injection, `SSL_get_error`, + `SSL_write`/`read` return overrides, and `BIO_set_flags`/`clear_flags` + spies. + +### Disagreements held + +- **Pushed back on the 5–10 s connect timeout** that came up during + socket-options review (Claude.ai). 200 ms is by design — Windows' + default `connect()` to a refused loopback retries internally for + ~2 s, throttling the BlockStore service-thread drain rate enough + that the discard policy never fires in the @windows_wip scenarios. + Kept 200 ms; flagged "tunable connect/handshake timeouts for WAN + deployments" as a follow-up story so far-cloud SIEMs aren't stuck + with the loopback-tuned default. + +- **Accepted 5 s for the TLS handshake budget**. Different concern + from the bare TCP connect — handshake takes 2–3 RTTs naturally; + my initial 200 ms plan was too tight for WAN. 5 s is the right + bound. + +### Deferred — three follow-up issues drafted in a parallel session + +- TCP keepalive + `SIO_KEEPALIVE_VALS` + `TCP_USER_TIMEOUT` for + dead-peer idle detection. Test-coverage decision (BDD with a + ~120 s scenario vs unit-only setsockopt assertions) explicitly + flagged in the issue body. +- TLS session resumption (`SSL_CTX_set_session_cache_mode` + + tickets) so frequent fail-fast reconnects don't pay the full + handshake cost. +- Tunable connect/handshake timeouts via CMake or + `SolidSyslogStreamSenderConfig`/`SolidSyslogTlsStreamConfig`, + for WAN deployments. + +### Local validation + +- All Linux gates green: gcc + clang builds, sanitize, tidy, cppcheck, + format, IWYU, coverage 100% (2024/2024 lines, 429/429 functions). +- MSVC: 973 unit tests pass, including the 50 Winsock TCP stream tests + that exercise the new non-blocking Send/Read contract. +- The four `@windows_wip` scenarios in `store_capacity.feature` were + spot-checked locally on the Windows MSVC build with otelcol-contrib + as the oracle. Scenario 1 reaches step 5 ("the syslog oracle + receives 1 message") successfully — confirming the architectural + decoupling unblocks the path. Step 6 ("stops accepting TCP + connections") fails locally because Docker Desktop's `wslrelay` / + `com.docker.backend` keep `5514` bound on `::` and `taskkill` only + affects `otelcol-contrib.exe`. Not a regression of S12.14; the + `@windows_wip` tag removal stays in PR #275's finale on the clean + windows-2025 CI runner. + +### Open questions + +- None for this PR; the three deferred stories cover the residue + surfaced during socket-options review. From 78b0cfeeaa3050a70641a863224345e003a3fca4 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 07:29:48 +0100 Subject: [PATCH 06/16] fix: silence tidy bugprone-easily-swappable-parameters on fcntl fake POSIX fcntl(int fd, int cmd, ...) has two adjacent int parameters by contract; we cannot change the signature in the link-substituted fake. NOLINTNEXTLINE on the existing readability-inconsistent suppression. --- Tests/Support/SocketFake.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tests/Support/SocketFake.c b/Tests/Support/SocketFake.c index 6b844d25..2c2d9038 100644 --- a/Tests/Support/SocketFake.c +++ b/Tests/Support/SocketFake.c @@ -804,7 +804,7 @@ ssize_t recv(int sockfd, void* buf, size_t len, int flags) /* fcntl is variadic in glibc; its third argument is an int when cmd is F_GETFL/ * F_SETFL (the only commands the production code uses). */ -// NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name) -- POSIX API +// NOLINTNEXTLINE(readability-inconsistent-declaration-parameter-name,bugprone-easily-swappable-parameters) -- POSIX API; signature fixed by libc int fcntl(int fd, int cmd, ...) { (void) fd; From b751844c2b9bfd187713b172621c306da53d8909 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 07:53:25 +0100 Subject: [PATCH 07/16] refactor: replace #ifdef sleep with platform-injected SolidSyslogSleepFunction SKILL.md bans #ifdef in production code. The TlsStream's bounded handshake retry sleep is now an integrator-injected callback (SolidSyslogSleepFunction typedef in Core/Interface/SolidSyslogSleep.h). Two platform implementations ship alongside the rest of the platform-specific helpers: - SolidSyslogPosixSleep wraps nanosleep. - SolidSyslogWindowsSleep wraps Sleep. The example TLS-sender wirings inject the platform impl (PosixSleep on the Linux Threaded example, WindowsSleep on the Windows example). Unit tests inject a local NoOpSleep through the config field instead of UT_PTR_SET on a function-pointer seam, so SolidSyslogTlsStreamInternal.h is no longer needed and is deleted. SOLIDSYSLOG_TLS_STREAM_SIZE bumped from 13 to 14 intptr_t slots to account for the new config field. --- CLAUDE.md | 3 +++ Core/Interface/SolidSyslogSleep.h | 16 +++++++++++ .../ExampleTlsSender_OpenSsl_PosixTcp.c | 2 ++ .../ExampleTlsSender_OpenSsl_WinsockTcp.c | 2 ++ .../OpenSsl/Interface/SolidSyslogTlsStream.h | 4 ++- .../OpenSsl/Source/SolidSyslogTlsStream.c | 27 +++---------------- .../Source/SolidSyslogTlsStreamInternal.h | 17 ------------ Platform/Posix/CMakeLists.txt | 1 + .../Posix/Interface/SolidSyslogPosixSleep.h | 14 ++++++++++ Platform/Posix/Source/SolidSyslogPosixSleep.c | 16 +++++++++++ Platform/Windows/CMakeLists.txt | 1 + .../Interface/SolidSyslogWindowsSleep.h | 14 ++++++++++ .../Windows/Source/SolidSyslogWindowsSleep.c | 8 ++++++ Tests/CMakeLists.txt | 3 ++- .../SolidSyslogTlsStreamIntegrationTest.cpp | 10 +++++++ Tests/SolidSyslogPosixSleepTest.cpp | 17 ++++++++++++ Tests/SolidSyslogTlsStreamTest.cpp | 20 +------------- Tests/SolidSyslogWindowsSleepTest.cpp | 15 +++++++++++ 18 files changed, 128 insertions(+), 62 deletions(-) create mode 100644 Core/Interface/SolidSyslogSleep.h delete mode 100644 Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h create mode 100644 Platform/Posix/Interface/SolidSyslogPosixSleep.h create mode 100644 Platform/Posix/Source/SolidSyslogPosixSleep.c create mode 100644 Platform/Windows/Interface/SolidSyslogWindowsSleep.h create mode 100644 Platform/Windows/Source/SolidSyslogWindowsSleep.c create mode 100644 Tests/SolidSyslogPosixSleepTest.cpp create mode 100644 Tests/SolidSyslogWindowsSleepTest.cpp diff --git a/CLAUDE.md b/CLAUDE.md index d999fd0b..0bcfb13e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -358,10 +358,13 @@ live under `Core/Interface/`; platform-specific helpers (the `SolidSyslogPosix*` | `SolidSyslogPosixHostname.h` | String callback implementor using POSIX hostname | `SolidSyslogPosixHostname_Get` (writes into `SolidSyslogFormatter*`) | | `SolidSyslogPosixProcessId.h` | String callback implementor using POSIX process ID | `SolidSyslogPosixProcessId_Get` (writes into `SolidSyslogFormatter*`) | | `SolidSyslogPosixSysUpTime.h` | SysUpTime callback implementor using POSIX `CLOCK_BOOTTIME` | `SolidSyslogPosixSysUpTime_Get` (returns `uint32_t` hundredths since boot, wraps per RFC 3418 `TimeTicks`) | +| `SolidSyslogPosixSleep.h` | System setup code wiring a `SolidSyslogSleepFunction` on POSIX targets | `SolidSyslogPosixSleep` (wraps `nanosleep`) | | `SolidSyslogWindowsClock.h` | System setup code using Windows clock | `SolidSyslogWindowsClock_GetTimestamp` | | `SolidSyslogWindowsHostname.h` | String callback implementor using Windows hostname | `SolidSyslogWindowsHostname_Get` (writes into `SolidSyslogFormatter*`) | | `SolidSyslogWindowsProcessId.h` | String callback implementor using Windows process ID | `SolidSyslogWindowsProcessId_Get` (writes into `SolidSyslogFormatter*`) | | `SolidSyslogWindowsSysUpTime.h` | SysUpTime callback implementor using `GetTickCount64` | `SolidSyslogWindowsSysUpTime_Get` (returns `uint32_t` hundredths since boot), `WindowsSysUpTime_GetTickCount64` (function-pointer seam for unit tests) | +| `SolidSyslogWindowsSleep.h` | System setup code wiring a `SolidSyslogSleepFunction` on Windows targets | `SolidSyslogWindowsSleep` (wraps `Sleep`) | +| `SolidSyslogSleep.h` | Any code passing or implementing a sleep callback | `SolidSyslogSleepFunction` typedef (used by `SolidSyslogTlsStreamConfig.sleep` for the bounded handshake retry) | | `SolidSyslogStructuredData.h` | Library internals (SD dispatch) | `SolidSyslogStructuredData_Format` (writes into `SolidSyslogFormatter*`) | | `SolidSyslogStructuredDataDefinition.h` | SD implementors (extension point) | `SolidSyslogStructuredData` vtable struct (Format takes `SolidSyslogFormatter*`) | | `SolidSyslogMetaSd.h` | System setup code using meta SD (sequenceId, sysUpTime, language) | `SolidSyslogMetaSdConfig` (counter, getSysUpTime, getLanguage — each independently optional via NULL), `SolidSyslogSysUpTimeFunction`, `SolidSyslogMetaSd_Create`, `_Destroy` | diff --git a/Core/Interface/SolidSyslogSleep.h b/Core/Interface/SolidSyslogSleep.h new file mode 100644 index 00000000..71a4ba8d --- /dev/null +++ b/Core/Interface/SolidSyslogSleep.h @@ -0,0 +1,16 @@ +#ifndef SOLIDSYSLOGSLEEP_H +#define SOLIDSYSLOGSLEEP_H + +#include "ExternC.h" + +EXTERN_C_BEGIN + + /* Cross-platform millisecond sleep callback. The TLS handshake retry loop + (and any future wait-and-retry path) takes one of these via config so + the library never sees platform-specific sleep APIs and tests can + inject a no-op without conditional compilation. */ + typedef void (*SolidSyslogSleepFunction)(int milliseconds); + +EXTERN_C_END + +#endif /* SOLIDSYSLOGSLEEP_H */ diff --git a/Example/Common/ExampleTlsSender_OpenSsl_PosixTcp.c b/Example/Common/ExampleTlsSender_OpenSsl_PosixTcp.c index 83bd5d66..ce92ac66 100644 --- a/Example/Common/ExampleTlsSender_OpenSsl_PosixTcp.c +++ b/Example/Common/ExampleTlsSender_OpenSsl_PosixTcp.c @@ -3,6 +3,7 @@ #include "ExampleMtlsConfig.h" #include "ExampleTlsConfig.h" #include "ExampleTlsSender.h" +#include "SolidSyslogPosixSleep.h" #include "SolidSyslogPosixTcpStream.h" #include "SolidSyslogStreamSender.h" #include "SolidSyslogTlsStream.h" @@ -23,6 +24,7 @@ struct SolidSyslogSender* ExampleTlsSender_Create(struct SolidSyslogResolver* re static struct SolidSyslogTlsStreamConfig tlsStreamConfig; tlsStreamConfig = (struct SolidSyslogTlsStreamConfig) {0}; tlsStreamConfig.transport = underlyingStream; + tlsStreamConfig.sleep = SolidSyslogPosixSleep; if (mtls) { tlsStreamConfig.caBundlePath = ExampleMtlsConfig_GetCaBundlePath(); diff --git a/Example/Common/ExampleTlsSender_OpenSsl_WinsockTcp.c b/Example/Common/ExampleTlsSender_OpenSsl_WinsockTcp.c index 3ed8bbe5..ab74851d 100644 --- a/Example/Common/ExampleTlsSender_OpenSsl_WinsockTcp.c +++ b/Example/Common/ExampleTlsSender_OpenSsl_WinsockTcp.c @@ -5,6 +5,7 @@ #include "ExampleTlsSender.h" #include "SolidSyslogStreamSender.h" #include "SolidSyslogTlsStream.h" +#include "SolidSyslogWindowsSleep.h" #include "SolidSyslogWinsockTcpStream.h" struct SolidSyslogResolver; @@ -23,6 +24,7 @@ struct SolidSyslogSender* ExampleTlsSender_Create(struct SolidSyslogResolver* re static struct SolidSyslogTlsStreamConfig tlsStreamConfig; tlsStreamConfig = (struct SolidSyslogTlsStreamConfig) {0}; tlsStreamConfig.transport = underlyingStream; + tlsStreamConfig.sleep = SolidSyslogWindowsSleep; if (mtls) { tlsStreamConfig.caBundlePath = ExampleMtlsConfig_GetCaBundlePath(); diff --git a/Platform/OpenSsl/Interface/SolidSyslogTlsStream.h b/Platform/OpenSsl/Interface/SolidSyslogTlsStream.h index 17f91509..c31f7f3f 100644 --- a/Platform/OpenSsl/Interface/SolidSyslogTlsStream.h +++ b/Platform/OpenSsl/Interface/SolidSyslogTlsStream.h @@ -4,6 +4,7 @@ #include #include "ExternC.h" +#include "SolidSyslogSleep.h" struct SolidSyslogStream; @@ -11,7 +12,7 @@ EXTERN_C_BEGIN enum { - SOLIDSYSLOG_TLS_STREAM_SIZE = sizeof(intptr_t) * 13 + SOLIDSYSLOG_TLS_STREAM_SIZE = sizeof(intptr_t) * 14 }; typedef struct @@ -22,6 +23,7 @@ EXTERN_C_BEGIN struct SolidSyslogTlsStreamConfig { struct SolidSyslogStream* transport; /* underlying byte stream — caller owns */ + SolidSyslogSleepFunction sleep; /* drives bounded handshake retry between WANT_READ/WANT_WRITE polls — required */ const char* caBundlePath; /* PEM file of trust anchors */ const char* serverName; /* SNI + cert hostname check; NULL to skip */ const char* cipherList; /* TLS 1.2 cipher list; NULL = OpenSSL default */ diff --git a/Platform/OpenSsl/Source/SolidSyslogTlsStream.c b/Platform/OpenSsl/Source/SolidSyslogTlsStream.c index 7f841f2c..04bb492b 100644 --- a/Platform/OpenSsl/Source/SolidSyslogTlsStream.c +++ b/Platform/OpenSsl/Source/SolidSyslogTlsStream.c @@ -5,17 +5,10 @@ #include #include -#ifdef _WIN32 -#include -#else -#include -#endif - #include "SolidSyslogMacros.h" #include "SolidSyslogStream.h" #include "SolidSyslogStreamDefinition.h" #include "SolidSyslogTlsStream.h" -#include "SolidSyslogTlsStreamInternal.h" enum { @@ -27,20 +20,6 @@ enum HANDSHAKE_POLL_INTERVAL_MILLISECONDS = 1 }; -static void DefaultSleep(int milliseconds); - -TlsStreamSleepFn TlsStream_sleep = DefaultSleep; - -static void DefaultSleep(int milliseconds) -{ -#ifdef _WIN32 - Sleep((DWORD) milliseconds); -#else - struct timespec ts = {.tv_sec = milliseconds / 1000, .tv_nsec = (long) (milliseconds % 1000) * 1000000L}; - (void) nanosleep(&ts, NULL); -#endif -} - struct SolidSyslogAddress; struct SolidSyslogTlsStream @@ -82,11 +61,11 @@ static inline int TlsStream_TransportBioRead(BIO* bio, char* buffer static inline int TlsStream_TransportBioWrite(BIO* bio, const char* buffer, int size); static const struct SolidSyslogTlsStream DEFAULT_INSTANCE = { - {TlsStream_Open, TlsStream_Send, TlsStream_Read, TlsStream_Close}, {NULL, NULL, NULL, NULL, NULL, NULL}, NULL, NULL, NULL, + {TlsStream_Open, TlsStream_Send, TlsStream_Read, TlsStream_Close}, {NULL, NULL, NULL, NULL, NULL, NULL, NULL}, NULL, NULL, NULL, }; static const struct SolidSyslogTlsStream DESTROYED_INSTANCE = { - {NULL, NULL, NULL, NULL}, {NULL, NULL, NULL, NULL, NULL, NULL}, NULL, NULL, NULL, + {NULL, NULL, NULL, NULL}, {NULL, NULL, NULL, NULL, NULL, NULL, NULL}, NULL, NULL, NULL, }; struct SolidSyslogStream* SolidSyslogTlsStream_Create(SolidSyslogTlsStreamStorage* storage, const struct SolidSyslogTlsStreamConfig* config) @@ -381,7 +360,7 @@ static inline bool TlsStream_PerformHandshake(struct SolidSyslogTlsStream* strea } else { - TlsStream_sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS); + stream->config.sleep(HANDSHAKE_POLL_INTERVAL_MILLISECONDS); totalSleptMs += HANDSHAKE_POLL_INTERVAL_MILLISECONDS; } } diff --git a/Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h b/Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h deleted file mode 100644 index 6b9ac92e..00000000 --- a/Platform/OpenSsl/Source/SolidSyslogTlsStreamInternal.h +++ /dev/null @@ -1,17 +0,0 @@ -#ifndef SOLIDSYSLOGTLSSTREAMINTERNAL_H -#define SOLIDSYSLOGTLSSTREAMINTERNAL_H - -#include "ExternC.h" - -EXTERN_C_BEGIN - - /* Function-pointer seam for the TLS handshake retry loop. Production wires - this to a platform-conditional sleep (Sleep on Windows, nanosleep on - POSIX); tests UT_PTR_SET it to a no-op so the bounded retry budget - drains in microseconds rather than its real wall-clock duration. */ - typedef void (*TlsStreamSleepFn)(int milliseconds); - extern TlsStreamSleepFn TlsStream_sleep; - -EXTERN_C_END - -#endif /* SOLIDSYSLOGTLSSTREAMINTERNAL_H */ diff --git a/Platform/Posix/CMakeLists.txt b/Platform/Posix/CMakeLists.txt index 3fa2f4ed..20c8e64b 100644 --- a/Platform/Posix/CMakeLists.txt +++ b/Platform/Posix/CMakeLists.txt @@ -5,6 +5,7 @@ target_sources(${PROJECT_NAME} PRIVATE Source/SolidSyslogPosixMutex.c Source/SolidSyslogPosixProcessId.c Source/SolidSyslogPosixSysUpTime.c + Source/SolidSyslogPosixSleep.c Source/SolidSyslogAddress.c Source/SolidSyslogGetAddrInfoResolver.c Source/SolidSyslogPosixDatagram.c diff --git a/Platform/Posix/Interface/SolidSyslogPosixSleep.h b/Platform/Posix/Interface/SolidSyslogPosixSleep.h new file mode 100644 index 00000000..f3e9ceab --- /dev/null +++ b/Platform/Posix/Interface/SolidSyslogPosixSleep.h @@ -0,0 +1,14 @@ +#ifndef SOLIDSYSLOGPOSIXSLEEP_H +#define SOLIDSYSLOGPOSIXSLEEP_H + +#include "ExternC.h" + +EXTERN_C_BEGIN + + /* POSIX implementation of SolidSyslogSleepFunction. Wraps nanosleep so + the TLS handshake retry loop yields to the kernel between attempts. */ + void SolidSyslogPosixSleep(int milliseconds); + +EXTERN_C_END + +#endif /* SOLIDSYSLOGPOSIXSLEEP_H */ diff --git a/Platform/Posix/Source/SolidSyslogPosixSleep.c b/Platform/Posix/Source/SolidSyslogPosixSleep.c new file mode 100644 index 00000000..724b4153 --- /dev/null +++ b/Platform/Posix/Source/SolidSyslogPosixSleep.c @@ -0,0 +1,16 @@ +#include "SolidSyslogPosixSleep.h" + +#include + +enum +{ + MILLISECONDS_PER_SECOND = 1000, + NANOSECONDS_PER_MILLISECOND = 1000000L +}; + +void SolidSyslogPosixSleep(int milliseconds) +{ + struct timespec ts = {.tv_sec = milliseconds / MILLISECONDS_PER_SECOND, + .tv_nsec = (long) (milliseconds % MILLISECONDS_PER_SECOND) * NANOSECONDS_PER_MILLISECOND}; + (void) nanosleep(&ts, NULL); +} diff --git a/Platform/Windows/CMakeLists.txt b/Platform/Windows/CMakeLists.txt index ce72230a..5ae2bb2c 100644 --- a/Platform/Windows/CMakeLists.txt +++ b/Platform/Windows/CMakeLists.txt @@ -11,6 +11,7 @@ if(HAVE_WINDOWS_PLATFORM) Source/SolidSyslogWindowsHostname.c Source/SolidSyslogWindowsMutex.c Source/SolidSyslogWindowsProcessId.c + Source/SolidSyslogWindowsSleep.c Source/SolidSyslogWindowsSysUpTime.c ) endif() diff --git a/Platform/Windows/Interface/SolidSyslogWindowsSleep.h b/Platform/Windows/Interface/SolidSyslogWindowsSleep.h new file mode 100644 index 00000000..3f9fadd9 --- /dev/null +++ b/Platform/Windows/Interface/SolidSyslogWindowsSleep.h @@ -0,0 +1,14 @@ +#ifndef SOLIDSYSLOGWINDOWSSLEEP_H +#define SOLIDSYSLOGWINDOWSSLEEP_H + +#include "ExternC.h" + +EXTERN_C_BEGIN + + /* Windows implementation of SolidSyslogSleepFunction. Wraps Sleep so the + TLS handshake retry loop yields to the scheduler between attempts. */ + void SolidSyslogWindowsSleep(int milliseconds); + +EXTERN_C_END + +#endif /* SOLIDSYSLOGWINDOWSSLEEP_H */ diff --git a/Platform/Windows/Source/SolidSyslogWindowsSleep.c b/Platform/Windows/Source/SolidSyslogWindowsSleep.c new file mode 100644 index 00000000..72faa6df --- /dev/null +++ b/Platform/Windows/Source/SolidSyslogWindowsSleep.c @@ -0,0 +1,8 @@ +#include "SolidSyslogWindowsSleep.h" + +#include + +void SolidSyslogWindowsSleep(int milliseconds) +{ + Sleep((DWORD) milliseconds); +} diff --git a/Tests/CMakeLists.txt b/Tests/CMakeLists.txt index 55f708db..3b65e6a4 100644 --- a/Tests/CMakeLists.txt +++ b/Tests/CMakeLists.txt @@ -80,6 +80,7 @@ if(SOLIDSYSLOG_POSIX) SolidSyslogPosixClockTest.cpp SolidSyslogPosixMessageQueueBufferTest.cpp SolidSyslogPosixMutexTest.cpp + SolidSyslogPosixSleepTest.cpp SolidSyslogPosixSysUpTimeTest.cpp ClockFakeTest.cpp SolidSyslogUdpSenderTest.cpp @@ -111,6 +112,7 @@ if(HAVE_WINDOWS_PLATFORM) SolidSyslogWindowsHostnameTest.cpp SolidSyslogWindowsMutexTest.cpp SolidSyslogWindowsProcessIdTest.cpp + SolidSyslogWindowsSleepTest.cpp SolidSyslogWindowsSysUpTimeTest.cpp ) endif() @@ -141,7 +143,6 @@ endif() if(SOLIDSYSLOG_OPENSSL) target_link_libraries(${PROJECT_NAME}Tests PRIVATE OpenSslFakes) - target_include_directories(${PROJECT_NAME}Tests PRIVATE ${CMAKE_SOURCE_DIR}/Platform/OpenSsl/Source) endif() if(HAVE_WINDOWS_PLATFORM OR SOLIDSYSLOG_WINSOCK) diff --git a/Tests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cpp b/Tests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cpp index c34fe487..3e655ac6 100644 --- a/Tests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cpp +++ b/Tests/OpenSslIntegration/SolidSyslogTlsStreamIntegrationTest.cpp @@ -15,6 +15,15 @@ #include "TlsTestServer.h" #include "CppUTest/TestHarness.h" +/* BioPairStream pumps synchronously — SSL_connect completes in one call so + * the handshake retry loop never sleeps. Provide a NoOp to satisfy the + * required config field without taking a platform dependency on the + * integration tests (these run on both POSIX and Windows). */ +static void NoOpSleep(int milliseconds) +{ + (void) milliseconds; +} + // clang-format off TEST_GROUP(TlsStreamIntegration) { @@ -80,6 +89,7 @@ TEST_GROUP(TlsStreamIntegration) BioPairStream_SetPump(transport, TlsTestServer_Pump, server); tlsConfig.transport = transport; + tlsConfig.sleep = NoOpSleep; tlsConfig.caBundlePath = caPath; tlsConfig.serverName = clientServerName; tlsStream = SolidSyslogTlsStream_Create(&tlsStreamStorage, &tlsConfig); diff --git a/Tests/SolidSyslogPosixSleepTest.cpp b/Tests/SolidSyslogPosixSleepTest.cpp new file mode 100644 index 00000000..c23d87a0 --- /dev/null +++ b/Tests/SolidSyslogPosixSleepTest.cpp @@ -0,0 +1,17 @@ +#include "SolidSyslogPosixSleep.h" +#include "CppUTest/TestHarness.h" + +// clang-format off +TEST_GROUP(SolidSyslogPosixSleep) +{ +}; +// clang-format on + +TEST(SolidSyslogPosixSleep, ReturnsImmediatelyForZero) +{ + /* nanosleep with a zero-length duration is a defined no-op return; the + test simply pins that the wrapper does not crash and returns under + any vaguely reasonable wall-clock budget. Sub-millisecond completion + is the expected behaviour on every supported POSIX platform. */ + SolidSyslogPosixSleep(0); +} diff --git a/Tests/SolidSyslogTlsStreamTest.cpp b/Tests/SolidSyslogTlsStreamTest.cpp index 1baefc9d..3e129779 100644 --- a/Tests/SolidSyslogTlsStreamTest.cpp +++ b/Tests/SolidSyslogTlsStreamTest.cpp @@ -8,7 +8,6 @@ #include "SolidSyslogAddress.h" #include "SolidSyslogStream.h" #include "SolidSyslogTlsStream.h" -#include "SolidSyslogTlsStreamInternal.h" #include "SolidSyslogTransport.h" #include "StreamFake.h" #include "CppUTest/TestHarness.h" @@ -37,9 +36,9 @@ TEST_GROUP(SolidSyslogTlsStream) OpenSslFake_Reset(); g_sleepCallCount = 0; g_lastSleepMs = 0; - UT_PTR_SET(TlsStream_sleep, NoOpSleep); transport = StreamFake_Create(); config.transport = transport; + config.sleep = NoOpSleep; // cppcheck-suppress unreadVariable -- used across TEST_GROUP methods; cppcheck does not model CppUTest macros stream = SolidSyslogTlsStream_Create(&streamStorage, &config); // cppcheck-suppress unreadVariable -- used across TEST_GROUP methods; cppcheck does not model CppUTest macros @@ -845,23 +844,6 @@ TEST(SolidSyslogTlsStream, DefaultPortMatchesRfc5425) LONGS_EQUAL(6514, SOLIDSYSLOG_TLS_DEFAULT_PORT); } -/* ------------------------------------------------------------------------- - * Default platform sleep — exercised without the no-op override so the - * production sleep helper is reached at least once for coverage. Calls with - * 0 ms keep the test fast. - * ------------------------------------------------------------------------- */ - -// clang-format off -TEST_GROUP(SolidSyslogTlsStreamDefaultSleep) -{ -}; -// clang-format on - -TEST(SolidSyslogTlsStreamDefaultSleep, DefaultSleepReturnsImmediatelyForZero) -{ - TlsStream_sleep(0); -} - /* ------------------------------------------------------------------------- * Non-blocking BIO read translation. Under the new transport contract a * 0 return means "would-block, retry"; without BIO_set_retry_read OpenSSL diff --git a/Tests/SolidSyslogWindowsSleepTest.cpp b/Tests/SolidSyslogWindowsSleepTest.cpp new file mode 100644 index 00000000..d6073f2a --- /dev/null +++ b/Tests/SolidSyslogWindowsSleepTest.cpp @@ -0,0 +1,15 @@ +#include "SolidSyslogWindowsSleep.h" +#include "CppUTest/TestHarness.h" + +// clang-format off +TEST_GROUP(SolidSyslogWindowsSleep) +{ +}; +// clang-format on + +TEST(SolidSyslogWindowsSleep, ReturnsImmediatelyForZero) +{ + /* Sleep(0) yields the remainder of the thread's quantum and returns + without blocking; the test pins that the wrapper does not crash. */ + SolidSyslogWindowsSleep(0); +} From 2f7ecfff45f1f15a3ddafd8891949718726640ee Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 08:26:53 +0100 Subject: [PATCH 08/16] fix: address CodeRabbit review on PR #283 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - SolidSyslogStream.h: Read doc now mentions "call Open before next Send or Read" (the < 0 path also closes the socket internally so a follow-up Read needs Open too). - SolidSyslog.c DrainBufferIntoStore: switch to using Store_Write's return value directly. The previous StoreDidNotRetainLastWrite() helper proxied via !HasUnsent(), which silently dropped a rejected Write into a non-empty store (e.g. BlockStore full + HALT discard policy). The bool contract is now explicit: true = retained for later replay; false = not held by this store. - SolidSyslogNullStore.c Write now returns false to honour that contract — NullStore never retains, so the eager-drain loop's !Write() branch takes the direct-send path. Preserves the constrained-system real-buffer + NullStore + UDP "one attempt per message, no buffering" wiring used by the Threaded example's --store=null mode. - StoreFake.c Write: queue-full now returns false and skips the writeCount bump so HasUnsent() and the test counters stay consistent with what was actually retained — mirrors the new real-store semantics. - SolidSyslogTest.cpp: lift the duplicated CircularBuffer + StoreFake + NullMutex setup out of the two new eager-drain tests into a SolidSyslogServiceEagerDrain TEST_GROUP. bufferStorage moves inside setup() with static storage duration so a CHECK failure that skips the test body's cleanup cannot leave a dangling stack reference behind for SolidSyslog_Destroy in teardown. - SocketFake.h: clarify the select() seam comment to spell out the three distinct simulations the fake supports — successful connect, deferred connect failure (writefds + SO_ERROR), and exceptfds rejection. --- Core/Interface/SolidSyslogStream.h | 2 +- Core/Source/SolidSyslog.c | 19 +++----- Core/Source/SolidSyslogNullStore.c | 6 ++- Tests/SolidSyslogNullStoreTest.cpp | 8 +++- Tests/SolidSyslogTest.cpp | 72 +++++++++++++++++------------- Tests/StoreFake.c | 14 +++--- Tests/Support/SocketFake.h | 16 +++++-- 7 files changed, 81 insertions(+), 56 deletions(-) diff --git a/Core/Interface/SolidSyslogStream.h b/Core/Interface/SolidSyslogStream.h index 30c655ae..d2388964 100644 --- a/Core/Interface/SolidSyslogStream.h +++ b/Core/Interface/SolidSyslogStream.h @@ -32,7 +32,7 @@ EXTERN_C_BEGIN > 0 bytes transferred into buffer = 0 nothing available right now (would-block) < 0 EOF or error — the underlying socket has been closed internally; the - caller must call Open before the next Send. */ + caller must call Open before the next Send or Read. */ SolidSyslogSsize SolidSyslogStream_Read(struct SolidSyslogStream * stream, void* buffer, size_t size); /* Close the underlying socket. Idempotent. */ diff --git a/Core/Source/SolidSyslog.c b/Core/Source/SolidSyslog.c index 9e84595d..e22afa3d 100644 --- a/Core/Source/SolidSyslog.c +++ b/Core/Source/SolidSyslog.c @@ -30,7 +30,6 @@ static inline bool CaptureTimestamp(struct SolidSyslogTimestamp* ts, Soli static inline uint8_t CombineFacilityAndSeverity(uint8_t facility, uint8_t severity); static inline bool FacilityIsValid(uint8_t facility); static inline void DrainBufferIntoStore(void); -static inline bool StoreDidNotRetainLastWrite(void); static inline void SendOneFromStore(void); static inline void FormatCapturedTimestamp(struct SolidSyslogFormatter* f, const struct SolidSyslogTimestamp* ts); static inline void FormatMessage(struct SolidSyslogFormatter* f, const struct SolidSyslogMessage* message); @@ -131,9 +130,12 @@ static void ProcessMessages(void) /* Eagerly drain the buffer so the producer-side shock absorber stays small while * the sender is slow or down — overflow then engages the store's discard policy - * rather than silently dropping at the buffer. When a write does not land in the - * store (NullStore configuration, or a store-write rejection), best-effort - * direct send keeps the message moving. */ + * rather than silently dropping at the buffer. When the store does not retain + * the message (NullStore configuration, or a store-write rejection — e.g. a + * full BlockStore under the HALT discard policy), the drain falls through to + * a best-effort direct send. The Store_Write contract is therefore: true = + * retained for later replay via ReadNextUnsent; false = not held by this + * store, the caller is on its own. */ static inline void DrainBufferIntoStore(void) { char buf[SOLIDSYSLOG_MAX_MESSAGE_SIZE]; @@ -141,20 +143,13 @@ static inline void DrainBufferIntoStore(void) while (SolidSyslogBuffer_Read(instance.buffer, buf, sizeof(buf), &len)) { - SolidSyslogStore_Write(instance.store, buf, len); - - if (StoreDidNotRetainLastWrite()) + if (!SolidSyslogStore_Write(instance.store, buf, len)) { SolidSyslogSender_Send(instance.sender, buf, len); } } } -static inline bool StoreDidNotRetainLastWrite(void) -{ - return !SolidSyslogStore_HasUnsent(instance.store); -} - static inline void SendOneFromStore(void) { char buf[SOLIDSYSLOG_MAX_MESSAGE_SIZE]; diff --git a/Core/Source/SolidSyslogNullStore.c b/Core/Source/SolidSyslogNullStore.c index 3f9fe100..2d07ed97 100644 --- a/Core/Source/SolidSyslogNullStore.c +++ b/Core/Source/SolidSyslogNullStore.c @@ -43,12 +43,16 @@ void SolidSyslogNullStore_Destroy(void) instance.base.GetUsedBytes = NULL; } +/* NullStore never retains. Returns false to signal "not held by this store" + * so the eager-drain loop in ProcessMessages takes the direct-send path — + * NullStore + real-buffer + UDP is the constrained-system "one attempt per + * message, no buffering" configuration. */ static bool Write(struct SolidSyslogStore* self, const void* data, size_t size) { (void) self; (void) data; (void) size; - return true; + return false; } static bool ReadNextUnsent(struct SolidSyslogStore* self, void* data, size_t maxSize, size_t* bytesRead) diff --git a/Tests/SolidSyslogNullStoreTest.cpp b/Tests/SolidSyslogNullStoreTest.cpp index 7b120faf..9ffe5e9a 100644 --- a/Tests/SolidSyslogNullStoreTest.cpp +++ b/Tests/SolidSyslogNullStoreTest.cpp @@ -35,9 +35,13 @@ TEST(SolidSyslogNullStore, ReadNextUnsentReturnsFalse) CHECK_FALSE(SolidSyslogStore_ReadNextUnsent(store, data, sizeof(data), &bytesRead)); } -TEST(SolidSyslogNullStore, WriteReturnsTrue) +TEST(SolidSyslogNullStore, WriteReturnsFalseToSignalNotRetained) { - CHECK_TRUE(SolidSyslogStore_Write(store, "hello", 5)); + /* The Store_Write contract reads "true = retained for later replay; false + * = not held". NullStore never retains, so reports false — the eager-drain + * loop in ProcessMessages then takes the direct-send fallback, preserving + * the constrained-system "one attempt per message, no buffering" path. */ + CHECK_FALSE(SolidSyslogStore_Write(store, "hello", 5)); } TEST(SolidSyslogNullStore, MarkSentDoesNotCrash) diff --git a/Tests/SolidSyslogTest.cpp b/Tests/SolidSyslogTest.cpp index d4f38d39..f67fe64c 100644 --- a/Tests/SolidSyslogTest.cpp +++ b/Tests/SolidSyslogTest.cpp @@ -1473,17 +1473,49 @@ TEST(SolidSyslog, ServiceDoesNotMarkSentWhenSendingFromBuffer) BufferFake_Destroy(); } -TEST(SolidSyslog, ServiceEagerlyDrainsBufferIntoStoreInOneTick) +/* Shared fixture for the eager-drain Service tests — both wire a real + * CircularBuffer (drives the multi-message-per-tick path) and a FIFO + * StoreFake. Storage is static so a CHECK failure that skips the test + * body's cleanup cannot leave a dangling stack reference behind for + * SolidSyslog_Destroy in teardown. */ +// clang-format off +TEST_GROUP(SolidSyslogServiceEagerDrain) { - static constexpr size_t BUFFER_BYTES = 256; - SolidSyslogCircularBufferStorage bufferStorage[SOLIDSYSLOG_CIRCULARBUFFER_STORAGE_SIZE_BYTES(BUFFER_BYTES)]; - SolidSyslogBuffer* circularBuffer = SolidSyslogCircularBuffer_Create(bufferStorage, sizeof(bufferStorage), SolidSyslogNullMutex_Create()); - SolidSyslogStore* fakeStore = StoreFake_Create(); - SolidSyslogConfig serviceConfig = {circularBuffer, fakeSender, nullptr, nullptr, nullptr, nullptr, fakeStore, nullptr, 0}; + static constexpr size_t BUFFER_BYTES = 256; - SolidSyslog_Destroy(); - SolidSyslog_Create(&serviceConfig); + struct SolidSyslogSender* fakeSender = nullptr; + struct SolidSyslogBuffer* circularBuffer = nullptr; + struct SolidSyslogStore* fakeStore = nullptr; + + void setup() override + { + static SolidSyslogCircularBufferStorage bufferStorage[ + SOLIDSYSLOG_CIRCULARBUFFER_STORAGE_SIZE_BYTES(BUFFER_BYTES)]; + + fakeSender = SenderFake_Create(); + circularBuffer = SolidSyslogCircularBuffer_Create( + bufferStorage, sizeof(bufferStorage), SolidSyslogNullMutex_Create()); + fakeStore = StoreFake_Create(); + + SolidSyslogConfig serviceConfig = {circularBuffer, fakeSender, nullptr, nullptr, + nullptr, nullptr, fakeStore, nullptr, 0}; + SolidSyslog_Create(&serviceConfig); + } + + void teardown() override + { + SolidSyslog_Destroy(); + StoreFake_Destroy(); + SolidSyslogCircularBuffer_Destroy(circularBuffer); + SolidSyslogNullMutex_Destroy(); + SenderFake_Destroy(fakeSender); + } +}; +// clang-format on + +TEST(SolidSyslogServiceEagerDrain, AllBufferedMessagesReachStoreInOneTickWhenSenderFails) +{ SolidSyslogBuffer_Write(circularBuffer, "msg1", 4); SolidSyslogBuffer_Write(circularBuffer, "msg2", 4); SolidSyslogBuffer_Write(circularBuffer, "msg3", 4); @@ -1491,30 +1523,14 @@ TEST(SolidSyslog, ServiceEagerlyDrainsBufferIntoStoreInOneTick) SolidSyslog_Service(); LONGS_EQUAL(3, StoreFake_WriteCount(fakeStore)); - - SolidSyslog_Destroy(); - SolidSyslog_Create(&config); - StoreFake_Destroy(); - SolidSyslogCircularBuffer_Destroy(circularBuffer); - SolidSyslogNullMutex_Destroy(); } -TEST(SolidSyslog, ServiceSendsStoredMessagesInFifoOrderAcrossTicks) +TEST(SolidSyslogServiceEagerDrain, StoredMessagesDrainInFifoOrderAcrossTicks) { - static constexpr size_t BUFFER_BYTES = 256; - SolidSyslogCircularBufferStorage bufferStorage[SOLIDSYSLOG_CIRCULARBUFFER_STORAGE_SIZE_BYTES(BUFFER_BYTES)]; - SolidSyslogBuffer* circularBuffer = SolidSyslogCircularBuffer_Create(bufferStorage, sizeof(bufferStorage), SolidSyslogNullMutex_Create()); - SolidSyslogStore* fakeStore = StoreFake_Create(); - SolidSyslogConfig serviceConfig = {circularBuffer, fakeSender, nullptr, nullptr, nullptr, nullptr, fakeStore, nullptr, 0}; - - SolidSyslog_Destroy(); - SolidSyslog_Create(&serviceConfig); - SolidSyslogBuffer_Write(circularBuffer, "m1", 2); SolidSyslogBuffer_Write(circularBuffer, "m2", 2); SolidSyslogBuffer_Write(circularBuffer, "m3", 2); - SenderFake_Reset(fakeSender); SolidSyslog_Service(); STRCMP_EQUAL("m1", SenderFake_LastBufferAsString(fakeSender)); SolidSyslog_Service(); @@ -1522,12 +1538,6 @@ TEST(SolidSyslog, ServiceSendsStoredMessagesInFifoOrderAcrossTicks) SolidSyslog_Service(); STRCMP_EQUAL("m3", SenderFake_LastBufferAsString(fakeSender)); LONGS_EQUAL(3, SenderFake_SendCount(fakeSender)); - - SolidSyslog_Destroy(); - SolidSyslog_Create(&config); - StoreFake_Destroy(); - SolidSyslogCircularBuffer_Destroy(circularBuffer); - SolidSyslogNullMutex_Destroy(); } TEST(SolidSyslog, ServiceDoesNothingWhenStoreIsHalted) diff --git a/Tests/StoreFake.c b/Tests/StoreFake.c index 5ac08605..806a257e 100644 --- a/Tests/StoreFake.c +++ b/Tests/StoreFake.c @@ -73,14 +73,18 @@ static bool Write(struct SolidSyslogStore* self, const void* data, size_t size) return false; } - if (fake->count < STOREFAKE_MAX_MESSAGES) + /* Queue full mirrors a real store's HALT-on-overflow: report the rejection + * via false and don't bump writeCount, so HasUnsent() and the test-side + * counters stay consistent with what was actually retained. */ + if (fake->count >= STOREFAKE_MAX_MESSAGES) { - size_t copySize = MinSize(size, STOREFAKE_MAX_MESSAGE_SIZE); - memcpy(fake->entries[fake->count], data, copySize); - fake->sizes[fake->count] = copySize; - fake->count++; + return false; } + size_t copySize = MinSize(size, STOREFAKE_MAX_MESSAGE_SIZE); + memcpy(fake->entries[fake->count], data, copySize); + fake->sizes[fake->count] = copySize; + fake->count++; fake->writeCount++; return true; } diff --git a/Tests/Support/SocketFake.h b/Tests/Support/SocketFake.h index e16518e0..72ea87b4 100644 --- a/Tests/Support/SocketFake.h +++ b/Tests/Support/SocketFake.h @@ -91,10 +91,18 @@ EXTERN_C_BEGIN int SocketFake_LastFcntlSetFlags(void); bool SocketFake_FcntlSetFlSetNonBlocking(void); - /* select configuration. ready=true makes select report fd writable in writefds; - ready=false leaves writefds empty (timeout). hasError=true sets fd in - exceptfds. returnValue overrides the int return (1 ready, 0 timeout, -1 error). - Default: returns 1 (one fd ready: writable, no error). */ + /* select configuration — three independent simulations: + (1) successful non-blocking-connect completion: SetSelectWritable(true) + plus SetSoError(0) — fd is writable, SO_ERROR is clear. + (2) deferred connect failure (typical "connection refused" path on + POSIX): SetSelectWritable(true) plus SetSoError(ECONNREFUSED) — + the fd appears writable, getsockopt(SO_ERROR) reveals the error. + (3) select() reporting fd in exceptfds: SetSelectError(true) — the + production path additionally rejects fds in the exception set + via FD_ISSET on errorSet, even though typical kernels surface + connect failures via simulation (2) rather than exceptfds. + SetSelectReturn overrides the syscall return value (1 ready, 0 timeout, + -1 syscall failure). Default: returns 1 (one fd ready, no error). */ void SocketFake_SetSelectWritable(bool ready); void SocketFake_SetSelectError(bool hasError); void SocketFake_SetSelectReturn(int value); From 5b70e4c70bf1c63dd38a7a5f40204cbee48c34ed Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 08:58:31 +0100 Subject: [PATCH 09/16] refactor: drop redundant HasUnsent guard in SendOneFromStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ReadNextUnsent already returns false on an empty store (NullStore and the real BlockStore both honour the contract), so the leading HasUnsent check in the && chain is dead weight. The remaining ReadNextUnsent && Send → MarkSent reads more directly. Co-Authored-By: Claude Opus 4.7 (1M context) --- Core/Source/SolidSyslog.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/Core/Source/SolidSyslog.c b/Core/Source/SolidSyslog.c index e22afa3d..8df997b4 100644 --- a/Core/Source/SolidSyslog.c +++ b/Core/Source/SolidSyslog.c @@ -155,8 +155,7 @@ static inline void SendOneFromStore(void) char buf[SOLIDSYSLOG_MAX_MESSAGE_SIZE]; size_t len = 0; - if (SolidSyslogStore_HasUnsent(instance.store) && SolidSyslogStore_ReadNextUnsent(instance.store, buf, sizeof(buf), &len) && - SolidSyslogSender_Send(instance.sender, buf, len)) + if (SolidSyslogStore_ReadNextUnsent(instance.store, buf, sizeof(buf), &len) && SolidSyslogSender_Send(instance.sender, buf, len)) { SolidSyslogStore_MarkSent(instance.store); } From 43b2a7378e843e26e24efc649a9d0c8d51a8b24f Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 08:58:39 +0100 Subject: [PATCH 10/16] refactor: name Winsock select nfds placeholder constant The literal 1 in WinsockTcpStream::WaitForConnectCompletion's select() call relied on a nearby comment for meaning. Lifting it to a named WINSOCK_NFDS_IGNORED enum constant moves the explanation to the definition site and makes the call read at one level of abstraction. Co-Authored-By: Claude Opus 4.7 (1M context) --- Platform/Windows/Source/SolidSyslogWinsockTcpStream.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c b/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c index 23fff8ce..48d84d8e 100644 --- a/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c +++ b/Platform/Windows/Source/SolidSyslogWinsockTcpStream.c @@ -97,7 +97,11 @@ enum 10 failing attempts cost 2 s instead of 20 s. */ CONNECT_TIMEOUT_MILLISECONDS = 200, MILLISECONDS_PER_SECOND = 1000, - MICROSECONDS_PER_MILLISECOND = 1000 + MICROSECONDS_PER_MILLISECOND = 1000, + /* Winsock ignores nfds (its fd_set is a literal array, not a bitmask), + but POSIX-portable callers must pass the highest fd + 1. Pass any + positive value to keep the call well-formed against either ABI. */ + WINSOCK_NFDS_IGNORED = 1 }; struct SolidSyslogWinsockTcpStream @@ -248,10 +252,7 @@ static bool WaitForConnectCompletion(SOCKET fd) struct timeval timeout = {.tv_sec = CONNECT_TIMEOUT_MILLISECONDS / MILLISECONDS_PER_SECOND, .tv_usec = (CONNECT_TIMEOUT_MILLISECONDS % MILLISECONDS_PER_SECOND) * MICROSECONDS_PER_MILLISECOND}; - /* nfds is ignored on Winsock (Windows uses fd_set as a literal array, not - a bitmask), but POSIX-portable callers must pass the highest fd + 1. - Pass a positive value to keep the call well-formed against either ABI. */ - int rc = WinsockTcpStream_select(1, NULL, &writeSet, &errorSet, &timeout); + int rc = WinsockTcpStream_select(WINSOCK_NFDS_IGNORED, NULL, &writeSet, &errorSet, &timeout); return (rc > 0) && FD_ISSET(fd, &writeSet) && !FD_ISSET(fd, &errorSet); } From 5b6100b94582c48902737dd5ed782628a49339d1 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 08:58:45 +0100 Subject: [PATCH 11/16] refactor: replace positional SolidSyslogConfig init with named assignments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The eager-drain TEST_GROUP used a 9-element positional brace initializer for SolidSyslogConfig — silently shifts if a new field is added to the config struct. Switch to value-init plus explicit named assignments so new fields default to zero/nullptr and the intent of each populated slot is visible at the call site. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/SolidSyslogTest.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Tests/SolidSyslogTest.cpp b/Tests/SolidSyslogTest.cpp index f67fe64c..b4a2d7fb 100644 --- a/Tests/SolidSyslogTest.cpp +++ b/Tests/SolidSyslogTest.cpp @@ -1497,8 +1497,10 @@ TEST_GROUP(SolidSyslogServiceEagerDrain) bufferStorage, sizeof(bufferStorage), SolidSyslogNullMutex_Create()); fakeStore = StoreFake_Create(); - SolidSyslogConfig serviceConfig = {circularBuffer, fakeSender, nullptr, nullptr, - nullptr, nullptr, fakeStore, nullptr, 0}; + SolidSyslogConfig serviceConfig = {}; + serviceConfig.buffer = circularBuffer; + serviceConfig.sender = fakeSender; + serviceConfig.store = fakeStore; SolidSyslog_Create(&serviceConfig); } From 8d2d2c977f3e11f4c527536d8e29af9fdfa5b172 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 08:59:04 +0100 Subject: [PATCH 12/16] refactor: extract TLS test helpers and intent-naming CHECK macros MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The non-blocking BIO, handshake retry, Send fail-fast, and Read non-blocking test groups added in S12.14 each repeated a 4-6 line arrangement: open + drive a fake's return + invoke the SUT + grab a callback pointer. Lift the patterns into TEST_GROUP member helpers (InvokeBioRead*, ArrangeHandshake*, OpenThenCauseSslWriteFailure, SendShortMessage, OpenThenReadWithSslReturnAndError) so each test reads as one or two sentences. Same treatment for repeated assertion shapes — five CHECK_* macros (CHECK_BIO_READ_RETRY_SIGNALLED, CHECK_BIO_RETRY_FLAGS_CLEARED, CHECK_SSL_SESSION_CLOSED, CHECK_TRANSPORT_CLOSED_ONCE etc.) name the intent of each multi-line check so the failing-line pointer still maps to the caller, mirroring the CHECK_PRIVAL convention from SolidSyslogTest.cpp. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/SolidSyslogTlsStreamTest.cpp | 193 ++++++++++++++++------------- 1 file changed, 110 insertions(+), 83 deletions(-) diff --git a/Tests/SolidSyslogTlsStreamTest.cpp b/Tests/SolidSyslogTlsStreamTest.cpp index 3e129779..495edf8d 100644 --- a/Tests/SolidSyslogTlsStreamTest.cpp +++ b/Tests/SolidSyslogTlsStreamTest.cpp @@ -50,10 +50,91 @@ TEST_GROUP(SolidSyslogTlsStream) SolidSyslogTlsStream_Destroy(stream); StreamFake_Destroy(transport); } + + /* Drive the registered BIO read callback with the given transport return — + collapses the open + set-return + grab-callback + invoke boilerplate. */ + [[nodiscard]] int InvokeBioReadWithTransportReturn(SolidSyslogSsize transportReturn) const + { + SolidSyslogStream_Open(stream, addr); + StreamFake_SetReadReturn(transport, transportReturn); + int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); + char buf[16]; + return readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); + } + + /* Drive the registered BIO write callback while the underlying transport + Send is configured to fail. */ + [[nodiscard]] int InvokeBioWriteWithFailingTransport() const + { + SolidSyslogStream_Open(stream, addr); + StreamFake_SetSendFails(transport, true); + int (*writeFn)(BIO*, const char*, int) = OpenSslFake_LastBioWriteCallback(); + const char msg[] = "hi"; + return writeFn(OpenSslFake_LastBioReturned(), msg, (int) sizeof(msg)); + } + + /* Arrange SSL_connect to first emit `wantError`, then succeed on the next + call — exercises the bounded handshake retry loop's progress path. */ + static void ArrangeHandshakeRetryThenSucceed(int wantError) + { + int seq[] = {-1, 1}; + OpenSslFake_SetConnectReturnSequence(seq, 2); + OpenSslFake_SetGetErrorReturn(wantError); + } + + /* Arrange SSL_connect to fail with `errorCode` on every call — used both + for the persistent-WANT (budget-exhausted) and hard-error paths. */ + static void ArrangePersistentHandshakeError(int errorCode) + { + int seq[] = {-1}; + OpenSslFake_SetConnectReturnSequence(seq, 1); + OpenSslFake_SetGetErrorReturn(errorCode); + } + + /* Open then arrange the next SSL_write to fail — exercises the Send fail-fast + teardown path that closes the SSL session and the underlying transport. */ + void OpenThenCauseSslWriteFailure() const + { + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetWriteFails(true); + } + + void SendShortMessage() const + { + const char msg[] = "hi"; + SolidSyslogStream_Send(stream, msg, sizeof(msg)); + } + + /* Open then arrange SSL_read to return the configured value while + SSL_get_error reports the configured SSL-level status — together they + exercise each branch of the Read non-blocking contract. */ + // NOLINTNEXTLINE(bugprone-easily-swappable-parameters) -- both ints, but name + comment make role distinct + [[nodiscard]] SolidSyslogSsize OpenThenReadWithSslReturnAndError(int sslReadReturn, int sslErrorCode) const + { + SolidSyslogStream_Open(stream, addr); + OpenSslFake_SetReadReturn(sslReadReturn); + OpenSslFake_SetGetErrorReturn(sslErrorCode); + char buf[16]; + return SolidSyslogStream_Read(stream, buf, sizeof(buf)); + } }; // clang-format on +// NOLINTBEGIN(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) +#define CHECK_BIO_READ_RETRY_SIGNALLED() LONGS_EQUAL(1, OpenSslFake_BioSetFlagsCallCount()) +#define CHECK_BIO_READ_RETRY_NOT_SIGNALLED() LONGS_EQUAL(0, OpenSslFake_BioSetFlagsCallCount()) +#define CHECK_BIO_RETRY_FLAGS_CLEARED() LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()) +#define CHECK_SSL_SESSION_CLOSED() \ + do \ + { \ + LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); \ + LONGS_EQUAL(1, OpenSslFake_FreeCallCount()); \ + } while (0) +#define CHECK_TRANSPORT_CLOSED_ONCE() LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)) + +// NOLINTEND(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) + TEST(SolidSyslogTlsStream, CreateSucceeds) { CHECK_TRUE(stream != nullptr); @@ -852,36 +933,21 @@ TEST(SolidSyslogTlsStream, DefaultPortMatchesRfc5425) TEST(SolidSyslogTlsStream, BioReadCallbackSignalsRetryWhenTransportWouldBlock) { - SolidSyslogStream_Open(stream, addr); - StreamFake_SetReadReturn(transport, 0); - int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); - char buf[16]; - int rc = readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); - LONGS_EQUAL(-1, rc); - LONGS_EQUAL(1, OpenSslFake_BioSetFlagsCallCount()); + LONGS_EQUAL(-1, InvokeBioReadWithTransportReturn(0)); + CHECK_BIO_READ_RETRY_SIGNALLED(); } TEST(SolidSyslogTlsStream, BioReadCallbackClearsRetryOnHardError) { - SolidSyslogStream_Open(stream, addr); - StreamFake_SetReadReturn(transport, -1); - int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); - char buf[16]; - int rc = readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); - LONGS_EQUAL(-1, rc); - LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()); + LONGS_EQUAL(-1, InvokeBioReadWithTransportReturn(-1)); + CHECK_BIO_RETRY_FLAGS_CLEARED(); } TEST(SolidSyslogTlsStream, BioReadCallbackReturnsBytesWhenTransportHasData) { - SolidSyslogStream_Open(stream, addr); - StreamFake_SetReadReturn(transport, 7); - int (*readFn)(BIO*, char*, int) = OpenSslFake_LastBioReadCallback(); - char buf[16]; - int rc = readFn(OpenSslFake_LastBioReturned(), buf, sizeof(buf)); - LONGS_EQUAL(7, rc); + LONGS_EQUAL(7, InvokeBioReadWithTransportReturn(7)); /* No retry signal needed: positive return is the success path. */ - LONGS_EQUAL(0, OpenSslFake_BioSetFlagsCallCount()); + CHECK_BIO_READ_RETRY_NOT_SIGNALLED(); } TEST(SolidSyslogTlsStream, BioWriteCallbackClearsRetryOnTransportFailure) @@ -889,13 +955,8 @@ TEST(SolidSyslogTlsStream, BioWriteCallbackClearsRetryOnTransportFailure) /* When the transport's fail-fast Send returns false the BIO must clear any stale retry flag and return -1 so OpenSSL surfaces SSL_ERROR_SYSCALL rather than spinning on a closed transport. */ - SolidSyslogStream_Open(stream, addr); - StreamFake_SetSendFails(transport, true); - int (*writeFn)(BIO*, const char*, int) = OpenSslFake_LastBioWriteCallback(); - const char msg[] = "hi"; - int rc = writeFn(OpenSslFake_LastBioReturned(), msg, (int) sizeof(msg)); - LONGS_EQUAL(-1, rc); - LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()); + LONGS_EQUAL(-1, InvokeBioWriteWithFailingTransport()); + CHECK_BIO_RETRY_FLAGS_CLEARED(); } /* ------------------------------------------------------------------------- @@ -906,19 +967,14 @@ TEST(SolidSyslogTlsStream, BioWriteCallbackClearsRetryOnTransportFailure) TEST(SolidSyslogTlsStream, OpenRetriesHandshakeOnWantRead) { - /* First call: WANT_READ; second call: success. */ - int seq[] = {-1, 1}; - OpenSslFake_SetConnectReturnSequence(seq, 2); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + ArrangeHandshakeRetryThenSucceed(SSL_ERROR_WANT_READ); CHECK_TRUE(SolidSyslogStream_Open(stream, addr)); LONGS_EQUAL(2, OpenSslFake_ConnectCallCount()); } TEST(SolidSyslogTlsStream, OpenSleepsBetweenHandshakeRetries) { - int seq[] = {-1, 1}; - OpenSslFake_SetConnectReturnSequence(seq, 2); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + ArrangeHandshakeRetryThenSucceed(SSL_ERROR_WANT_READ); SolidSyslogStream_Open(stream, addr); LONGS_EQUAL(1, g_sleepCallCount); } @@ -928,9 +984,7 @@ TEST(SolidSyslogTlsStream, OpenRetriesHandshakeOnWantWrite) /* WANT_WRITE arises when SSL needs to send (e.g. during the handshake finished message under non-blocking transport with a temporarily-full send buffer). Same retry treatment as WANT_READ. */ - int seq[] = {-1, 1}; - OpenSslFake_SetConnectReturnSequence(seq, 2); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_WRITE); + ArrangeHandshakeRetryThenSucceed(SSL_ERROR_WANT_WRITE); CHECK_TRUE(SolidSyslogStream_Open(stream, addr)); LONGS_EQUAL(2, OpenSslFake_ConnectCallCount()); } @@ -939,18 +993,14 @@ TEST(SolidSyslogTlsStream, OpenFailsWhenHandshakeNeverCompletes) { /* SSL_connect always returns -1 with WANT_READ — handshake never makes progress, so the bounded budget should expire and Open returns false. */ - int seq[] = {-1}; - OpenSslFake_SetConnectReturnSequence(seq, 1); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); + ArrangePersistentHandshakeError(SSL_ERROR_WANT_READ); CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); } TEST(SolidSyslogTlsStream, OpenFailsImmediatelyOnHardSslError) { /* Non-WANT error (e.g. SSL_ERROR_SSL) is fail-fast — no retry budget burn. */ - int seq[] = {-1}; - OpenSslFake_SetConnectReturnSequence(seq, 1); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_SSL); + ArrangePersistentHandshakeError(SSL_ERROR_SSL); CHECK_FALSE(SolidSyslogStream_Open(stream, addr)); LONGS_EQUAL(1, OpenSslFake_ConnectCallCount()); LONGS_EQUAL(0, g_sleepCallCount); @@ -963,21 +1013,16 @@ TEST(SolidSyslogTlsStream, OpenFailsImmediatelyOnHardSslError) TEST(SolidSyslogTlsStream, SendClosesSslOnWriteFailure) { - SolidSyslogStream_Open(stream, addr); - OpenSslFake_SetWriteFails(true); - const char msg[] = "hi"; - SolidSyslogStream_Send(stream, msg, sizeof(msg)); - LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); - LONGS_EQUAL(1, OpenSslFake_FreeCallCount()); + OpenThenCauseSslWriteFailure(); + SendShortMessage(); + CHECK_SSL_SESSION_CLOSED(); } TEST(SolidSyslogTlsStream, SendClosesTransportOnWriteFailure) { - SolidSyslogStream_Open(stream, addr); - OpenSslFake_SetWriteFails(true); - const char msg[] = "hi"; - SolidSyslogStream_Send(stream, msg, sizeof(msg)); - LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)); + OpenThenCauseSslWriteFailure(); + SendShortMessage(); + CHECK_TRANSPORT_CLOSED_ONCE(); } TEST(SolidSyslogTlsStream, SendReturnsFalseOnShortWrite) @@ -994,36 +1039,21 @@ TEST(SolidSyslogTlsStream, SendReturnsFalseOnShortWrite) TEST(SolidSyslogTlsStream, ReadReturnsZeroOnWantRead) { - SolidSyslogStream_Open(stream, addr); - OpenSslFake_SetReadReturn(-1); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_WANT_READ); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(0, n); + LONGS_EQUAL(0, OpenThenReadWithSslReturnAndError(-1, SSL_ERROR_WANT_READ)); } TEST(SolidSyslogTlsStream, ReadReturnsNegativeOneOnHardErrorAndClosesSsl) { - SolidSyslogStream_Open(stream, addr); - OpenSslFake_SetReadReturn(-1); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_SSL); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(-1, n); - LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); - LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)); + LONGS_EQUAL(-1, OpenThenReadWithSslReturnAndError(-1, SSL_ERROR_SSL)); + CHECK_SSL_SESSION_CLOSED(); + CHECK_TRANSPORT_CLOSED_ONCE(); } TEST(SolidSyslogTlsStream, ReadReturnsNegativeOneOnZeroReturnAndClosesSsl) { /* SSL_read returns 0 → SSL_ERROR_ZERO_RETURN (clean shutdown by peer). */ - SolidSyslogStream_Open(stream, addr); - OpenSslFake_SetReadReturn(0); - OpenSslFake_SetGetErrorReturn(SSL_ERROR_ZERO_RETURN); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(-1, n); - LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); + LONGS_EQUAL(-1, OpenThenReadWithSslReturnAndError(0, SSL_ERROR_ZERO_RETURN)); + CHECK_SSL_SESSION_CLOSED(); } /* ------------------------------------------------------------------------- @@ -1034,11 +1064,8 @@ TEST(SolidSyslogTlsStream, ReadReturnsNegativeOneOnZeroReturnAndClosesSsl) TEST(SolidSyslogTlsStream, CloseAfterInternalCloseFromSendFailureDoesNotDoubleFree) { - SolidSyslogStream_Open(stream, addr); - OpenSslFake_SetWriteFails(true); - const char msg[] = "hi"; - SolidSyslogStream_Send(stream, msg, sizeof(msg)); /* internal close */ - SolidSyslogStream_Close(stream); /* second close — must be safe */ - LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); - LONGS_EQUAL(1, OpenSslFake_FreeCallCount()); + OpenThenCauseSslWriteFailure(); + SendShortMessage(); /* internal close */ + SolidSyslogStream_Close(stream); /* second close — must be safe */ + CHECK_SSL_SESSION_CLOSED(); } From 8ee9da3efd5b74624c06dc3ed8af9f0db9c69559 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 08:59:13 +0100 Subject: [PATCH 13/16] refactor: extract Read16ByteBuffer helper and CHECK_SOCKET_CLOSED_ONCE The non-blocking-Read tests added in S12.14 each declared a 16-byte buffer and called SolidSyslogStream_Read inline; the close-fd assertions captured an openFd local before invoking the path that closes it and then asserted (count == 1, lastClosed == openFd). Lift each into the TEST_GROUP / file-local form: a Read16ByteBuffer member returning the ssize directly, and a CHECK_SOCKET_CLOSED_ONCE macro that pairs the count check with the matching-fd check. Same pattern applied to both the POSIX and Winsock test files for symmetry. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/SolidSyslogPosixTcpStreamTest.cpp | 47 ++++++++++++----------- Tests/SolidSyslogWinsockTcpStreamTest.cpp | 40 ++++++++++--------- 2 files changed, 46 insertions(+), 41 deletions(-) diff --git a/Tests/SolidSyslogPosixTcpStreamTest.cpp b/Tests/SolidSyslogPosixTcpStreamTest.cpp index cc522469..e1c9b91c 100644 --- a/Tests/SolidSyslogPosixTcpStreamTest.cpp +++ b/Tests/SolidSyslogPosixTcpStreamTest.cpp @@ -48,10 +48,26 @@ TEST_GROUP(SolidSyslogPosixTcpStream) { SolidSyslogPosixTcpStream_Destroy(stream); } + + [[nodiscard]] SolidSyslogSsize Read16ByteBuffer() const + { + char buf[16]; + return SolidSyslogStream_Read(stream, buf, sizeof(buf)); + } }; // clang-format on +// NOLINTBEGIN(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) +#define CHECK_SOCKET_CLOSED_ONCE() \ + do \ + { \ + LONGS_EQUAL(1, SocketFake_CloseCallCount()); \ + LONGS_EQUAL(SocketFake_SocketFd(), SocketFake_LastClosedFd()); \ + } while (0) + +// NOLINTEND(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) + TEST(SolidSyslogPosixTcpStream, CreateDestroyWorksWithoutCrashing) { } @@ -178,11 +194,9 @@ TEST(SolidSyslogPosixTcpStream, SendReturnsFalseOnEagain) TEST(SolidSyslogPosixTcpStream, SendClosesSocketOnFailure) { SolidSyslogStream_Open(stream, addr); - int openFd = SocketFake_SocketFd(); SocketFake_SetSendFails(true); SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN); - LONGS_EQUAL(1, SocketFake_CloseCallCount()); - LONGS_EQUAL(openFd, SocketFake_LastClosedFd()); + CHECK_SOCKET_CLOSED_ONCE(); } TEST(SolidSyslogPosixTcpStream, OpenClosesSocketOnConnectFailure) @@ -380,8 +394,7 @@ TEST(SolidSyslogPosixTcpStream, OpenClosesSocketOnSelectTimeout) SocketFake_SetSelectWritable(false); SocketFake_SetSelectReturn(0); SolidSyslogStream_Open(stream, addr); - LONGS_EQUAL(1, SocketFake_CloseCallCount()); - LONGS_EQUAL(SocketFake_SocketFd(), SocketFake_LastClosedFd()); + CHECK_SOCKET_CLOSED_ONCE(); } TEST(SolidSyslogPosixTcpStream, OpenFailsWhenSelectFlagsErrorOnFd) @@ -436,40 +449,28 @@ TEST(SolidSyslogPosixTcpStream, ReadReturnsZeroOnEagain) { SolidSyslogStream_Open(stream, addr); SocketFake_FailNextRecvWithErrno(EAGAIN); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(0, n); + LONGS_EQUAL(0, Read16ByteBuffer()); } TEST(SolidSyslogPosixTcpStream, ReadReturnsZeroOnWouldBlock) { SolidSyslogStream_Open(stream, addr); SocketFake_FailNextRecvWithErrno(EWOULDBLOCK); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(0, n); + LONGS_EQUAL(0, Read16ByteBuffer()); } TEST(SolidSyslogPosixTcpStream, ReadReturnsNegativeOneOnEofAndClosesSocket) { SolidSyslogStream_Open(stream, addr); - int openFd = SocketFake_SocketFd(); SocketFake_SetRecvReturn(0); /* EOF */ - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(-1, n); - LONGS_EQUAL(1, SocketFake_CloseCallCount()); - LONGS_EQUAL(openFd, SocketFake_LastClosedFd()); + LONGS_EQUAL(-1, Read16ByteBuffer()); + CHECK_SOCKET_CLOSED_ONCE(); } TEST(SolidSyslogPosixTcpStream, ReadReturnsNegativeOneOnErrorAndClosesSocket) { SolidSyslogStream_Open(stream, addr); - int openFd = SocketFake_SocketFd(); SocketFake_FailNextRecvWithErrno(ECONNRESET); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(-1, n); - LONGS_EQUAL(1, SocketFake_CloseCallCount()); - LONGS_EQUAL(openFd, SocketFake_LastClosedFd()); + LONGS_EQUAL(-1, Read16ByteBuffer()); + CHECK_SOCKET_CLOSED_ONCE(); } diff --git a/Tests/SolidSyslogWinsockTcpStreamTest.cpp b/Tests/SolidSyslogWinsockTcpStreamTest.cpp index ce80d426..a19682a5 100644 --- a/Tests/SolidSyslogWinsockTcpStreamTest.cpp +++ b/Tests/SolidSyslogWinsockTcpStreamTest.cpp @@ -54,10 +54,26 @@ TEST_GROUP(SolidSyslogWinsockTcpStream) { SolidSyslogWinsockTcpStream_Destroy(stream); } + + [[nodiscard]] SolidSyslogSsize Read16ByteBuffer() const + { + char buf[16]; + return SolidSyslogStream_Read(stream, buf, sizeof(buf)); + } }; // clang-format on +// NOLINTBEGIN(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) +#define CHECK_SOCKET_CLOSED_ONCE() \ + do \ + { \ + LONGS_EQUAL(1, WinsockFake_CloseCallCount()); \ + CHECK(WinsockFake_SocketFd() == WinsockFake_LastClosedFd()); \ + } while (0) + +// NOLINTEND(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) + TEST(SolidSyslogWinsockTcpStream, CreateDestroyWorksWithoutCrashing) { } @@ -317,11 +333,9 @@ TEST(SolidSyslogWinsockTcpStream, SendDoesNotRetryAfterShortWrite) TEST(SolidSyslogWinsockTcpStream, SendClosesSocketOnFailure) { SolidSyslogStream_Open(stream, addr); - SOCKET openFd = WinsockFake_SocketFd(); WinsockFake_SetSendFails(true); SolidSyslogStream_Send(stream, TEST_MESSAGE, TEST_MESSAGE_LEN); - LONGS_EQUAL(1, WinsockFake_CloseCallCount()); - CHECK(openFd == WinsockFake_LastClosedFd()); + CHECK_SOCKET_CLOSED_ONCE(); } TEST(SolidSyslogWinsockTcpStream, CloseCallsCloseOnce) @@ -397,33 +411,23 @@ TEST(SolidSyslogWinsockTcpStream, ReadReturnsZeroOnWouldBlock) { SolidSyslogStream_Open(stream, addr); WinsockFake_FailNextRecvWithLastError(WSAEWOULDBLOCK); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(0, n); + LONGS_EQUAL(0, Read16ByteBuffer()); } TEST(SolidSyslogWinsockTcpStream, ReadReturnsNegativeOneOnEofAndClosesSocket) { SolidSyslogStream_Open(stream, addr); - SOCKET openFd = WinsockFake_SocketFd(); WinsockFake_SetRecvReturn(0); /* EOF */ - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(-1, n); - LONGS_EQUAL(1, WinsockFake_CloseCallCount()); - CHECK(openFd == WinsockFake_LastClosedFd()); + LONGS_EQUAL(-1, Read16ByteBuffer()); + CHECK_SOCKET_CLOSED_ONCE(); } TEST(SolidSyslogWinsockTcpStream, ReadReturnsNegativeOneOnErrorAndClosesSocket) { SolidSyslogStream_Open(stream, addr); - SOCKET openFd = WinsockFake_SocketFd(); WinsockFake_FailNextRecvWithLastError(WSAECONNRESET); - char buf[16]; - SolidSyslogSsize n = SolidSyslogStream_Read(stream, buf, sizeof(buf)); - LONGS_EQUAL(-1, n); - LONGS_EQUAL(1, WinsockFake_CloseCallCount()); - CHECK(openFd == WinsockFake_LastClosedFd()); + LONGS_EQUAL(-1, Read16ByteBuffer()); + CHECK_SOCKET_CLOSED_ONCE(); } TEST(SolidSyslogWinsockTcpStream, DestroyClosesOpenSocket) From 80d9f1105a3e98a5bc2f6466eaaf24ab5ad87958 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 09:10:40 +0100 Subject: [PATCH 14/16] fix: add IWYU forward declarations for two TLS test classes IWYU reported missing forward declarations for the two test bodies that reference the new CHECK_TRANSPORT_CLOSED_ONCE macro (ReadReturnsNegativeOneOnHardErrorAndClosesSsl, SendClosesTransportOnWriteFailure). Same forward-decl pattern as other test files in the repo. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/SolidSyslogTlsStreamTest.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Tests/SolidSyslogTlsStreamTest.cpp b/Tests/SolidSyslogTlsStreamTest.cpp index 495edf8d..52db5182 100644 --- a/Tests/SolidSyslogTlsStreamTest.cpp +++ b/Tests/SolidSyslogTlsStreamTest.cpp @@ -12,6 +12,9 @@ #include "StreamFake.h" #include "CppUTest/TestHarness.h" +class TEST_SolidSyslogTlsStream_ReadReturnsNegativeOneOnHardErrorAndClosesSsl_Test; +class TEST_SolidSyslogTlsStream_SendClosesTransportOnWriteFailure_Test; + static int g_sleepCallCount; static int g_lastSleepMs; From f094709a9708830cd8c8a536130133cc4b8a7c20 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 09:24:40 +0100 Subject: [PATCH 15/16] refactor: address CodeRabbit review on TLS test macros - Wrap the four single-statement CHECK_* macros in `do { ... } while (0)` for consistency with CHECK_SSL_SESSION_CLOSED and the codebase convention. Guards against the if/else dangling-statement pitfall if the macro is ever extended. - Add CHECK_TRANSPORT_CLOSED_ONCE() to the SSL_ERROR_ZERO_RETURN test so it locks down the full Read fail-fast contract on the clean-shutdown path, matching the hard-error variant. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/SolidSyslogTlsStreamTest.cpp | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/Tests/SolidSyslogTlsStreamTest.cpp b/Tests/SolidSyslogTlsStreamTest.cpp index 52db5182..4dc92eb3 100644 --- a/Tests/SolidSyslogTlsStreamTest.cpp +++ b/Tests/SolidSyslogTlsStreamTest.cpp @@ -125,16 +125,32 @@ TEST_GROUP(SolidSyslogTlsStream) // clang-format on // NOLINTBEGIN(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) -#define CHECK_BIO_READ_RETRY_SIGNALLED() LONGS_EQUAL(1, OpenSslFake_BioSetFlagsCallCount()) -#define CHECK_BIO_READ_RETRY_NOT_SIGNALLED() LONGS_EQUAL(0, OpenSslFake_BioSetFlagsCallCount()) -#define CHECK_BIO_RETRY_FLAGS_CLEARED() LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()) +#define CHECK_BIO_READ_RETRY_SIGNALLED() \ + do \ + { \ + LONGS_EQUAL(1, OpenSslFake_BioSetFlagsCallCount()); \ + } while (0) +#define CHECK_BIO_READ_RETRY_NOT_SIGNALLED() \ + do \ + { \ + LONGS_EQUAL(0, OpenSslFake_BioSetFlagsCallCount()); \ + } while (0) +#define CHECK_BIO_RETRY_FLAGS_CLEARED() \ + do \ + { \ + LONGS_EQUAL(1, OpenSslFake_BioClearFlagsCallCount()); \ + } while (0) #define CHECK_SSL_SESSION_CLOSED() \ do \ { \ LONGS_EQUAL(1, OpenSslFake_ShutdownCallCount()); \ LONGS_EQUAL(1, OpenSslFake_FreeCallCount()); \ } while (0) -#define CHECK_TRANSPORT_CLOSED_ONCE() LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)) +#define CHECK_TRANSPORT_CLOSED_ONCE() \ + do \ + { \ + LONGS_EQUAL(1, StreamFake_CloseCallCount(transport)); \ + } while (0) // NOLINTEND(cppcoreguidelines-macro-usage,cppcoreguidelines-avoid-do-while) @@ -1057,6 +1073,7 @@ TEST(SolidSyslogTlsStream, ReadReturnsNegativeOneOnZeroReturnAndClosesSsl) /* SSL_read returns 0 → SSL_ERROR_ZERO_RETURN (clean shutdown by peer). */ LONGS_EQUAL(-1, OpenThenReadWithSslReturnAndError(0, SSL_ERROR_ZERO_RETURN)); CHECK_SSL_SESSION_CLOSED(); + CHECK_TRANSPORT_CLOSED_ONCE(); } /* ------------------------------------------------------------------------- From d8dbe07309c59c0233564d9461f096d8b4f7e026 Mon Sep 17 00:00:00 2001 From: David Cozens Date: Thu, 7 May 2026 09:29:17 +0100 Subject: [PATCH 16/16] fix: add IWYU forward decl for ZERO_RETURN test The previous commit added CHECK_TRANSPORT_CLOSED_ONCE() to the ReadReturnsNegativeOneOnZeroReturnAndClosesSsl test. Touching the TEST_GROUP `transport` member via that macro triggers the same IWYU forward-decl pattern as the other two transport-touching tests in this file. Co-Authored-By: Claude Opus 4.7 (1M context) --- Tests/SolidSyslogTlsStreamTest.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/SolidSyslogTlsStreamTest.cpp b/Tests/SolidSyslogTlsStreamTest.cpp index 4dc92eb3..7e778908 100644 --- a/Tests/SolidSyslogTlsStreamTest.cpp +++ b/Tests/SolidSyslogTlsStreamTest.cpp @@ -13,6 +13,7 @@ #include "CppUTest/TestHarness.h" class TEST_SolidSyslogTlsStream_ReadReturnsNegativeOneOnHardErrorAndClosesSsl_Test; +class TEST_SolidSyslogTlsStream_ReadReturnsNegativeOneOnZeroReturnAndClosesSsl_Test; class TEST_SolidSyslogTlsStream_SendClosesTransportOnWriteFailure_Test; static int g_sleepCallCount;