Repository navigation
llmessage and llcorehttp: fix the five bugs their tests' follow-up found - #458
Merged
Merged
Conversation
…t byte splitAfter meant to leave a segment whole when asked to split it after its last byte, but tested for the address one past it, which getSegment never returns. So it split anyway and inserted the zero-length segment its comment says it avoids. LLBufferStreamBuf::sync had come to rely on that. It splits after the last byte written and erases the segment after the split, the part of the put area the stream did not use. When a write had filled its segment, the segment it erased was the empty one, so it erased nothing. With the split fixed it would erase whatever came next, another channel's data included, on every flush and on the stream's destruction. It now trims only a put area with something left over. llbuffer's tests check that a split after the last byte adds nothing, and io's that a flush after filling the put segment keeps the segment after it. Each fails without its half of the fix. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
seekoff clamped a read position to the last byte, so a seek to the end left the next read returning that byte rather than EOF, and a putback there putting back the byte before it. It also took an offset from the end the wrong way, so seekg(-2, end) went two bytes past the end and was clamped back, and it let a seek before the start wrap round as a size_t. Over an empty BufferArray every seek failed, the end being one byte before the start. A seek from the current position moved the read position even when the seek then failed. Both directions now work out where a seek lands the same way: from the start, the current position or the end, adding the offset; before the start fails and changes nothing, past the end is the end. A read position at the end lies just past the last byte, in that byte's block. Nothing in the viewer seeks one of these streams, so this changes no behaviour there. bufferstream's tests check the end, an offset from it, a write from it, a seek before the start, and an empty BufferArray. The seek to the end now puts back the '!' that the TUT test's message said it would, where the check had expected EOF. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
HTTP_RETRY_BACKOFF_MAX_DEFAULT is commented "5 sec" and was 5E6L when MAINT-7495 set it, to stop the viewer retrying for too long. A 2024 change that wrote these constants as integers gave it as 50000006UL, fifty seconds. A request that sets no ceiling of its own has backed off since as if it had none: with the default five retries, after 1, 2, 4, 8 and 16 seconds instead of 1, 2, 4, 5 and 5. HttpOptions takes the same default, clamped on its way into a request to the 20-second limit, which is still above every one of those delays. It is 5 seconds again. Two static_asserts hold the defaults in order and within that limit; the second fails on the old value. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LLDataPackerAsciiBuffer's and LLDataPackerAsciiFile's unpackBinaryData took the size a line gave, capped only by the caller's buffer, and read that many bytes three characters at a time from the line, a 512-byte copy on the stack, with no regard to where the line ended. A line that claimed more bytes than it held was read on past its terminator and past the copy, and the unpack reported success with the size it claimed. A line whose size did not parse left out_size as it was, which LLTextureAnim, among others, leaves uninitialised. unpackBinaryDataFixed read the caller's count from its line the same way. Both packers now share one unpack that stops where the line does. A line with no size fails with out_size 0; one that holds fewer bytes than it claims gives what it holds, fails, and says how many bytes it gave in out_size. unpackBinaryDataFixed fails likewise when its line is short. What a well-formed line unpacks to is unchanged. lldatapacker's tests give each a line that claims more than it holds, and one with no size. They fail on the old code, which reported success for every one of them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
addNode refuses a node for a path that already has one and leaves it with its caller, but said nothing, and buildAllServices passed it a node fresh from a factory and forgot it. Building the services into a root that has some of them already, as building twice does, leaked a node a path. addNode now returns whether it took the node, and buildAllServices deletes one it refused. llhttpnode's tests check the refusal through the return value, and that a second buildAllServices into the same root leaves as many nodes alive as the first: it registers a node type that counts itself. Both fail without the fix. Not changed: a second wildcard under one parent, named differently from the first, replaces it rather than being refused, and the first is leaked. Refusing it would change which node answers such a path. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 27 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
Description
Fixes the five bugs in llmessage's and llcorehttp's own code that #456 found while fixing their tests and left alone, since that change was to tests only. Each fix comes with a test that fails without it.
It stacks on #456 (
rye/tm19-test-fixes), whose Catch2 tests these extend.LLBufferArray::splitAfter.getSegmentnever returns. So it split anyway and inserted the zero-length segment its comment says it avoids.LLBufferStreamBuf::synchad come to rely on that. It splits after the last byte written and erases the segment after the split, which is the unused rest of the put area. When a write had filled its segment, what it erased was the empty segment. With the split fixed alone, it would erase whatever came next, another channel's data included, on every flush and every stream destruction. It now trims only a put area that has something left over.BufferArrayStreamBuf::seekoff.size_t;BufferArrayevery seek failed;'!', as the original TUT message said it should.HTTP_RETRY_BACKOFF_MAX_DEFAULT.5E6Lwhen MAINT-7495 set it. A 2024 change that rewrote these constants as integers typed50000006UL, fifty seconds.static_asserts keep the defaults in order and within the 20-second limit; the second fails on the old value.unpackBinaryDatatook the size a line gave and read that many bytes from the 512-byte copy of the line, wherever the line ended. It reported success with the size the line claimed. A line whose size did not parse leftout_sizeas it was, whichLLTextureAnimleaves uninitialised.unpackBinaryDataFixedread past a short line the same way.out_sizesays how much it gave.LLHTTPRegistrar::buildAllServices.addNoderefuses a node for a path that already has one and leaves the node with its caller, but it said nothing.buildAllServicesforgot the node it had just built, leaking one for each path already present.addNodereturns whether it took the node, andbuildAllServicesdeletes a node it refused.Related Issues
Issue Link: none. These were found by #456 and listed in its notes.
Checklist
Additional Notes
ctest -LE manualpass, and so do the 8 pending combined runs;llmessage_testsandllcorehttp_testseach pass 10 random orders, with no state guard report.lldatapacker.cpp, and failed. Each other fix was undone in part: the split,sync's guard, each part ofseekoff, the backoff value, the delete inbuildAllServices, andaddNode's refusal. Each time the test meant for it failed.addNodegiven a second wildcard under one parent, named differently from the first, replaces the first and leaks it rather than refusing it. Refusing it would change which node answers such paths, so it wants its own change.🤖 Generated with Claude Code