Test PR for on-demand benchmark workflow - #7
Open
roshkhatri wants to merge 2 commits into
Open
Conversation
|
On-Demand Benchmark ran on commit: RPS Benchmark Comparison: unstable vs d798669No significant changesNo statistically significant changes detected across 1 test(s). 1 with no significant change Click to expand full comparison tables
Configuration:
Legend:
Statistical Notes:
|
roshkhatri
pushed a commit
that referenced
this pull request
Aug 10, 2026
) ### Problem The bug was originally introduced by valkey-io#1737 In tls.c, we normally avoid consolidating IO vectors into a buffer if they exceed a capped amount (`NET_MAX_WRITES_PER_EVENT`, which is 64KB). However, on OpenSSL write errors, we must never trigger a subsequent write that is smaller than the previous failed write. In this path, we must consolidate the IO vectors to ensure the write is at least as large as the last write we made on the socket: ```c char buf[iov_bytes_len]; size_t offset = 0; for (int i = 0; i < iovcnt && offset < iov_bytes_len; i++) { memcpy(buf + offset, iov[i].iov_base, iov[i].iov_len); ``` However, `iov_bytes_len` can easily exceed the stack boundaries if one of the IO vectors is large (e.g. a large key read), leading to a crash: ``` Thread 1 (Thread 0x7ffff7b65700 (LWP 191) "valkey-server"): #0 0x00005555557a04bc in connTLSWritev (conn_=0x7ffff6e518c0, iov=<optimized out>, iovcnt=5) at src/tls.c:1713 #1 0x00005555556d2a21 in connWritev (conn=<optimized out>, iov=0x7fffffff9240, iovcnt=5) at src/connection.h:256 #2 writevToClient (c=<optimized out>) at src/networking.c:2814 #3 _writeToClient (c=<optimized out>) at src/networking.c:2868 #4 0x00005555556d326c in writeToClient (c=0x7ffff6e7a780) at src/networking.c:3117 #5 writeToClient (c=0x7ffff6e7a780) at src/networking.c:3108 #6 sendReplyToClient (conn=<optimized out>) at src/networking.c:3127 #7 0x00005555557a0ffd in callHandler (conn=0x7ffff6e518c0, handler=<optimized out>) at src/connhelpers.h:79 #8 tlsHandleEvent (conn=0x7ffff6e518c0, mask=<optimized out>) at src/tls.c:1507 #9 0x00005555555fbad5 in aeProcessEvents (flags=27, eventLoop=0x7ffff6e45f80) at src/ae.c:504 #10 aeMain (eventLoop=0x7ffff6e45f80) at src/ae.c:543 #11 0x00005555555eb83a in main (argc=2, argv=0x7fffffffd588) at src/server.c:7833 ``` ### Fix To fix this, we simply avoid allocating large buffers on the stack by using a fixed-size stack buffer `char buf[NET_MAX_WRITES_PER_EVENT]` (64KB) and performing partial copies: 1. Calculate total length of all `iov` blocks to decide if we can use one-by-one writing. 2. If total length is larger than `NET_MAX_WRITES_PER_EVENT` (64KB) and `iov[0]` is large enough to satisfy `last_failed_write_data_len`, write blocks sequentially one-by-one. 3. Otherwise (combine path), copy data from `iov` into a fixed-size stack buffer of `NET_MAX_WRITES_PER_EVENT` (64KB), capping the copy at 64KB (partial copy if it exceeds). 4. Verify that we copied at least `last_failed_write_data_len` to satisfy OpenSSL retry constraint. 5. Write the combined buffer and return the number of bytes copied (Valkey connection layer will handle the remaining data as a partial write retry). Important: this should keep the existing "write must be larger than last failed write" invariant, considering: 1) if the last write went to the consolidated buffer path, it would be at most `NET_MAX_WRITES_PER_EVENT` 2) if the last write did not go through the consolidated buffer path, it will be at the front of the `iov`, and therefore our `iov[0] >= last_failed_write_data_len` check will always pass (it is the same write). Finally, since we added IO threaded TLS writes, the stack limit may be even smaller for the thread (depending on the system). We work around this in bio threads, and I am copying that workaround to IO threads to ensure the stack is not smaller than `NET_MAX_WRITES_PER_EVENT`. ### Testing A regression test was appended to the existing TLS test suite in tests/unit/tls.tcl. We need to add a new debug subcommand `DEBUG FORCE-TLS-WRITE-ERROR <0 or 1>` to deterministically trigger OpenSSL write failures. With this flow, we write a command with a small response and force it to trigger an IO error, then we write a command with a larger response and resolve the IO errors. The response to the large command will accumulate in the buffer, and we will attempt to write out the full output buffer for the client, which now includes a massive final IO vector. In the old code, this would trigger a stack overflow as we try to allocate O(megabytes) on the stack. --------- Signed-off-by: Jacob Murphy <jkmurphy@google.com> Co-authored-by: Ran Shidlansik <ranshid@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Dummy PR to test the on-demand benchmark workflow.