[bugfix] BinaryValueFromFile: honor shared-reference reference counting#6506
Open
joewiz wants to merge 1 commit into
Open
[bugfix] BinaryValueFromFile: honor shared-reference reference counting#6506joewiz wants to merge 1 commit into
joewiz wants to merge 1 commit into
Conversation
187d96c to
289b228
Compare
BinaryValueFromFile did not honor the shared-reference reference counting
that XQueryContext.enterEnclosedExpr()/exitEnclosedExpr() rely on:
incrementSharedReferences() was a no-op and close() released the FileChannel
unconditionally. So when a file-backed binary value was used in an element
(or document) constructor -- an enclosed expression -- exitEnclosedExpr()
closed the value's channel immediately after the constructor, and any later
read of the value failed with "Underlying channel has been closed". For
example (count($w) forces the constructor to be evaluated before $b is read
again):
let $b := file:read-binary($path)
let $w := <a>{$b}</a>
return (count($w), $b)[2]
BinaryValueFromInputStream was unaffected because it already reference-counts
through AbstractFilterInputStreamCache (close() releases only when the shared
reference count reaches zero).
Root cause confirmed by instrumentation: the close fires from
XQueryContext.exitEnclosedExpr() right after the constructor reads the value;
the failing read is the subsequent use of the value. It reproduces on both
the embedded and REST execution paths -- only the consumer of the second read
differs (the caller vs the response serializer).
Fix: give BinaryValueFromFile the same reference counting -- a counter
starting at 1, incremented by incrementSharedReferences() and decremented by
close(), releasing the channel/file handle only when it reaches zero. Eager
cleanup of values not shared out of a scope is preserved.
Tests:
- BinaryValueFromFileSharedReferenceTest (unit): models
enterEnclosedExpr()/exitEnclosedExpr() on a shared file-backed value; it
must stay open and readable, then be released by the final close.
- AbstractBinariesTest#readBinaryUsedInElementConstructorThenReadAgain
(file module): the query above, run over BOTH the embedded
(EmbeddedBinariesTest) and REST (RestBinariesTest) execution paths; both
fail with "Underlying channel has been closed" before the fix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
289b228 to
33d7cde
Compare
4 tasks
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.
[This PR was prompted by Joe, drafted by Claude Code, and reviewed by Joe.]
Summary
A file-backed binary value (
BinaryValueFromFile, e.g. fromfile:read-binaryorrequest:get-uploaded-file-data) that is used in an element or document constructor (an enclosed expression) and then read again could have its file channel closed underneath it, after which the second read failed with "Underlying channel has been closed":Root cause
BinaryValueFromFiledid not honor the shared-reference reference counting thatXQueryContext.enterEnclosedExpr()/exitEnclosedExpr()rely on:incrementSharedReferences()was a no-op ("we don't need reference counting…"), andclose()released theFileChannel/RandomAccessFileunconditionally.enterEnclosedExpr()increments the shared-reference count of each live binary value so a value that escapes an enclosed expression survives; the matchingexitEnclosedExpr()thenclose()s each, intending only to decrement a reference. ForBinaryValueFromFilethe increment did nothing and theclose()actually tore down the channel — so a value used in a constructor was closed immediately after the constructor, while it was still referenced.BinaryValueFromInputStreamwas unaffected because it already reference-counts throughAbstractFilterInputStreamCache.Confirmed by instrumentation: the close fires from
XQueryContext.exitEnclosedExpr()right after the constructor reads the value, and the failing read is the subsequent use of the value.Fix
Give
BinaryValueFromFilethe same reference counting as its input-stream sibling: a counter starting at 1, incremented byincrementSharedReferences()and decremented byclose(), releasing the channel/file handle only when it reaches zero. Eager cleanup of values not shared out of a scope is preserved (their count goes 1 → 0 on cleanup).What changed
exist-core/.../xquery/value/BinaryValueFromFile.java— add thesharedReferencescounter;incrementSharedReferences()increments it;close()decrements and releases the channel only at zero (idempotent once released).Test plan
BinaryValueFromFileSharedReferenceTest(unit): modelsenterEnclosedExpr()/exitEnclosedExpr()on a shared file-backed value; it must stay open and readable, then be released by the finalclose. Fails before the fix, passes after.AbstractBinariesTest#readBinaryUsedInElementConstructorThenReadAgain(file module): the summary query, exercised over both the embedded (EmbeddedBinariesTest) and REST (RestBinariesTest) execution paths; both fail with "Underlying channel has been closed" before the fix and pass after.The regression test runs a main-module query (not an XQSuite test function) on purpose: inside a module function the bug is masked, because
ModuleContext.registerBinaryValueInstance()delegates registration to the parent/root context, whileenterEnclosedExpr()/exitEnclosedExpr()operate on theModuleContext's own (empty) deque — so the constructor'sexitEnclosedExpr()never sees the binary there and the close happens later viapopLocalVariables(after the read), harmlessly. The bug surfaces only where the binary and the enclosed expression share a context, i.e., in a main-module query. (That asymmetry inModuleContextis pre-existing and merely masks this bug; this PR does not change it.)Notes
xmldb:storefailure actually has a distinct root cause (Sequence.containsReferencenot recursing into nested map/array items, across five sequence types), now fixed separately in [bugfix] Sequence.containsReference: recurse into nested map/array items #6507. This PR fixes the enclosed-expression premature-close path – a sibling "file-backed binary closed while still referenced" bug.