Skip to content

fix(linkbuffer): expose the flush node from GetBytes(nil) after head drain - #430

Open
linhongyu510 wants to merge 1 commit into
cloudwego:mainfrom
linhongyu510:fix/getbytes-expose-flush-node
Open

linhongyu510 wants to merge 1 commit into
cloudwego:mainfrom
linhongyu510:fix/getbytes-expose-flush-node

Conversation

@linhongyu510

Copy link
Copy Markdown

What type of PR is this?

  • fix: A bug fix

Which issue(s) this PR fixes

None filed previously.

Background

UnsafeLinkBuffer.GetBytes(p) documents: "If p is not passed, return all readable bytes." (PR #313 added the len(p) == 0 auto-size path to implement exactly this.)

When p is empty/nil, the auto-size branch sized the result slice to the number of nodes strictly between read and flush:

n := 0
for ; node != flush; node = node.next { n++ }
p = make([][]byte, n)

The subsequent loop then fills non-empty intermediate nodes and finally appends the flush node only if i < len(p). Once read has advanced past the leading empty/unmanaged head node (i.e. after the first real read/skip), every node between read and flush is non-empty, so i == len(p) and the flush node is silently dropped.

Two concrete failure shapes:

  1. read == flush (the steady state after the empty head drains): the counting loop never runs, n = 0, p is empty, and the final if i < len(p) guard is 0 < 0 — so GetBytes(nil) returns nothing at all, even though Len() > 0 and Bytes() returns all the data.
  2. read != flush with several non-empty nodes: GetBytes(nil) returns the intermediate nodes but drops the most recently flushed node.

The existing TestLinkBufferGetBytes did not catch this because it never reads anything, so read stays parked on the empty head node, which happened to reserve a slot for the flush node.

Fix

When auto-sizing, count the non-empty nodes from read up to and including flush, so the flush node always has a reserved slot. This makes GetBytes(nil) content-equivalent to Bytes() (which already special-cases read == flush). The caller-provided p path (used internally with barriercap slots) is unchanged.

Regression

TestLinkBufferGetBytesExposesAllAfterHeadDrain asserts GetBytes(nil) reconstructs exactly Bytes() for both shapes. On the unpatched tree it fails:

read==flush: GetBytes(nil)=0 bytes "" want "a0Bb1Cc2Dd3" (Len=11)
read!=flush: GetBytes(nil)=44 bytes "...Nn3Oo4" want "...Nn3Oo4Pp5Qq6Rr7Ss8Tt9" (Len=59)

and passes with the fix.

Testing

  • go test ./... (norace): ok
  • go test -race ./...: ok
  • go vet ./...: clean
  • cross-compile: linux/amd64, linux/arm64 OK

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants