[
https://issues.apache.org/jira/browse/TIKA-4793?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18104392#comment-18104392
]
ASF GitHub Bot commented on TIKA-4793:
--------------------------------------
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.
```
> Make the Pipes IPC max payload size configurable (currently hard-coded to 100
> MB)
> ---------------------------------------------------------------------------------
>
> Key: TIKA-4793
> URL: https://issues.apache.org/jira/browse/TIKA-4793
> Project: Tika
> Issue Type: Improvement
> Components: tika-pipes
> Reporter: Srinivasarao Daruna
> Priority: Major
>
> PipesMessage.MAX_PAYLOAD_BYTES (tika-pipes-core) is a compile-time constant
> set to 100 MB:
> //
> tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/protocol/PipesMessage.java:44
> public static final int MAX_PAYLOAD_BYTES = 100 * 1024 * 1024;
> This cap is enforced on the read side of every IPC message in the
> PipesClient↔PipesServer socket protocol. It does not limit the file size
> being parsed (files are fetched server-side by a Fetcher); it limits the size
> of the serialized JSON payload — most critically the PipesResult (parsed
> metadata + extracted text) returned in FINISHED messages.
> Problems with the current implementation:
> 1. Hard-coded, not configurable. Users with very large documents that produce
> large parse results (and no MetadataWriteLimiterFactory configured) have no
> way to raise the cap short of forking the code. There is no corresponding
> field in PipesConfig.
> 2. No write-side guard. PipesMessage.write() applies no limit before writing.
> When the server serializes a PipesResult exceeding 100 MB and sends it, the
> client's PipesMessage.read() throws IOException("Payload length X exceeds
> maximum of 104857600 bytes"). This is caught by the catch-all Exception block
> in PipesClient.waitForServer() and surfaced to the caller as
> UNSPECIFIED_CRASH — a misleading status that provides no indication of the
> root cause.
> Proposed fix:
> 1. Add maxIpcPayloadBytes to PipesConfig with a default of 100 * 1024 * 1024,
> loaded from the "pipes" JSON config section (consistent with all other
> PipesConfig fields).
> 2. Thread the configured value through to both PipesMessage.read() and
> PipesMessage.write(), replacing the hard-coded constant.
> 3. Add a write-side guard in PipesMessage.write() so oversized results are
> caught server-side with a descriptive IOException rather than failing
> silently at the client with UNSPECIFIED_CRASH.
> Example config (proposed):
> {
> "pipes": {
> "maxIpcPayloadBytes": 209715200
> }
> }
> Note: Users hitting this limit should first consider configuring a
> MetadataWriteLimiterFactory to bound extracted-text size, which is the right
> long-term solution for very large documents. But the limit should still be
> configurable for cases where the full content is legitimately needed.
> Affected files:
> -
> tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/protocol/PipesMessage.java
> -
> tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PipesConfig.java
> -
> tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/PipesClient.java
> -
> tika-pipes/tika-pipes-core/src/main/java/org/apache/tika/pipes/core/server/PipesServer.java
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
