tballison commented on PR #3009:
URL: https://github.com/apache/tika/pull/3009#issuecomment-5279514561
Updated review. Let me know if any of these points do not check out.
```
Verdict
The BoundedOutputStream rewrite (19c6b1dc4f) is the right architecture and
resolves most of the earlier feedback — 7 of 9 items including Nicholas's
fallback-frame issue. But it's
not mergeable yet: two blockers you've already flagged, one confirmed
data-loss regression vs main, and the PR body/title now describe code that
doesn't exist.
Blockers
1. writeCrash — confirmed by reproduction, not just analysis
(ServerProtocolIO.java:166-175). Truncates to limit/2 chars then calls
unbounded toBytes() — the only write path that
bypasses the BoundedOutputStream. Smile is UTF-8, so ≥U+0800 chars encode
at 3 bytes/char. Repro: limit=1024, CJK exception message → 1498-byte frame →
client throws
PayloadLimitExceededException, and the server hung in awaitAck() until
connection close. Downstream, the crash is misreported as
PAYLOAD_LIMIT_EXCEEDED (TASK_EXCEPTION category)
instead of OOM/TIMEOUT (PROCESS_CRASH) — wrong category for retry logic,
crash detail lost. Javadoc claims "always fits." No test covers it. Fix: route
through the same
BoundedOutputStream+fallback as writeFinished, pin with a test.
2. OOM swallowed post-serialization. writeFinished now allocates up to ~3×
limit transiently (BAOS doubling + toByteArray() copy) on the main-loop thread.
An OOM there isn't an
IOException, so it lands in pre-existing catch (Throwable) blocks
(PipesServer.java:404, ConnectionHandler.java:183) and the JVM keeps running
post-OOM — per-client mode loops on;
shared mode stays corrupted for all clients. Violates fork-and-die.
Pre-existing catch sites, but this PR makes the allocation reachable, so it
should carry the fix (rethrow
Error/exit-on-OOM there, and/or -XX:+ExitOnOutOfMemoryError on the forked
JVM). Side note: buf.writeTo(output) instead of toByteArray() would drop one
full copy.
Confirmed regression vs main
3. Oversized docs vanish from audit output with
emitIntermediateResults=true. The static fallback has null emitData/emitKey. On
main, the client-side rejection path went through
buildFatalResult, attaching emitKey + intermediate metadata, so a failure
record was emitted. Now AsyncEmitter.add warn-skips the null emitData — no
trace of the document in emitter
output (PipesClient.java:418-424, AsyncEmitter.java:107-113). Fix: in the
FINISHED branch, when status is PAYLOAD_LIMIT_EXCEEDED with null emitData,
rebuild via buildFatalResult like
the exception path.
Your #1 (success clobbering) — still open, structurally foreclosed
Oversized EMIT_SUCCESS_PASSBACK/EMIT_SUCCESS_PARSE_EXCEPTION (already
emitted to S3/ES) still gets replaced wholesale by the failure-category
fallback → double-emit on retry. The
pre-serialized static frame can't carry the original status, so "degrade
emitData only, keep the status" was never implemented. Fix: retry serialization
with emitData stripped but
status kept; static frame only if even that overflows. (Not a regression
vs main, but it was the ask.)
Diagnosability (usability)
The new path gives the operator less than the old teardown did: the WARN
logs only the configured limit — no doc/emit key, no actual size (the
BoundedOutputStream knows it at abort)
— and nothing anywhere names the maxIpcPayloadBytes knob. Old path at
least logged "length X exceeds maximum Y" with the doc id. Cheap fix: enrich
the WARN; serialize a per-doc
message when the limit has headroom. Also: shared-server mode has
independently configured client/server limits — server limit > client limit
reproduces the old teardown; docs don't
say to keep them in sync.
Docs / hygiene
- PR body and title are stale: they describe the abandoned three-layer
guard, an "archive sizing fix" (×2→×1) that was reverted (final code keeps ×2;
only real change is 2→2L
overflow widening), and a test plan listing tests that don't exist. Author
should rewrite both.
- configuration.adoc:158 -Xmx sentence is backwards: says "lower -Xmx to
~3× this value"; should be "at least ~3×."
- CHANGES.txt: no TIKA-4793 entry; both the knob and the behavior change
(clean status vs teardown) belong there.
- Validation floor admits guaranteed-broken values (floor = 67-byte
fallback frame; anything below a typical FetchEmitTuple makes every request die
as undiagnosable
UNSPECIFIED_CRASH) — a pragmatic floor or load-time WARN would close the
footgun the docs currently just describe.
- awaitAck() still reads with the hardcoded 100 MB default
(ServerProtocolIO.java:184) — harmless for empty ACK frames, inconsistent with
the bidirectional contract.
- Minor: BoundedOutputStream int-overflow guard ((long) buf.size() + len),
dead serializedSize() helper + stale "estimate formula" comment in the test,
stray blank line in
TikaPipesConfigTest, writeIntermediate javadoc promises a "full result"
FINISHED that will almost certainly be the fallback.
What checks out
Rebase is clean (merge-base = current main, real diff 9 files +357/−28);
no estimate-based pre-check or OOM-catch remains in serialization; overflow
aborts before any wire byte, no
desync possible on the bounded paths; exactly-at-limit is symmetric
writer/reader; Nicholas's minimum-limit fix is complete (enforced in both
setter and constructor, config load goes
through the setter, test now reads with the configured limit);
PAYLOAD_LIMIT_EXCEEDED is handled sanely by tika-server, tika-grpc,
AsyncProcessor; single-threaded use of
ServerProtocolIO per connection, no sync issues; config can't disable the
guard.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
