[
https://issues.apache.org/jira/browse/TIKA-4815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18104777#comment-18104777
]
ASF GitHub Bot commented on TIKA-4815:
--------------------------------------
tballison commented on PR #3022:
URL: https://github.com/apache/tika/pull/3022#issuecomment-5293229947
This is a really important fix, and you caught a subtle bug in PipesClient.
Thank you.
Human feedback: the backpressure we added to tika-server via 429 is a huge
improvement to tika-server. What's the grpc native equivalent to 429 or is it
proto specific?
This is what my agent came up with. Let me know what you think.
Thank you again.
```
Ranked findings
1. [HIGH — found independently by correctness + security, mechanism
verified by me] Interrupt path omits markServerForRestart()
PipesClient.java:229-238. This is the only closeConnection() call site in
the file not paired with markServerForRestart() — the OOM, TIMEOUT, CRASH,
SocketTimeout and backstop branches all pair them.
In the default mode (useSharedServer=false): the orphaned forked JVM is
still alive mid-parse, pendingRestart was never set, so ensureRunning() returns
early
treating it as healthy (PerClientServerManager.java:238). But that process
already spent its one outbound connect and will never dial back, so connect()
blocks
the full hardcoded 60s on accept(), then throws
ServerInitializationException — which maybeInit() rethrows without retry →
FAILED_TO_INITIALIZE. Every
subsequent call on that pool slot repeats the 60s tax until the orphan
dies on its own. With numClients=1 a single interrupted call degrades all
traffic.
This directly contradicts the fix's own comment: it removes a stale-ping
stall and introduces a same-sized stale-accept stall.
The fix is not a blind added line — in shared mode markServerForRestart()
force-kills a server other clients are actively using. It needs to be
mode-aware (e.g.
a ServerManager method that PerClientServerManager implements as
pendingRestart = true and SharedServerManager treats as a no-op, since its
ConnectionHandler
already handles abandoned connections via clientGone).
2. [MEDIUM — 2 reviewers] The fix doesn't cover maybeInit()
PipesClient.java:209-221 catches only
ServerInitializationException/SecurityException. An interrupt during the
reconnect backoff Thread.sleep() escapes
process() entirely with connectionTuple pointing at a half-handshaked
socket, which PipesParser's unconditional finally re-queues. Self-healing (next
borrower's
ping fails fast), but it's the same gap the PR set out to close.
3. [MEDIUM — maintainer decision] Both new gRPC failure modes are
invisible server-side
Pool exhaustion returns CLIENT_UNAVAILABLE_WITHIN_MS with no log anywhere
— compare PipesParsingHelper.java:344, which logs it and maps to HTTP 429
specifically
so alerting can tell "at capacity" from "worker crashing." Separately,
fetchAndParseImpl:332 swallows InterruptedException with no onNext/onError;
that's
pre-existing, but the PR adds a brand-new 60s interruptible poll() that
makes it materially more reachable. Server-streaming callers see a successfully
completed empty stream; bidi callers silently lose one reply with no
correlation id to notice.
4. [LOW] Dead code the PR added
closeConnection() declares throws InterruptedException but provably never
throws it, so the new inner catch (InterruptedException e2) at :235-237 is
unreachable.
5. [LOW] Docs/CHANGES don't match behavior
- CHANGES claims a pooled client "cannot go back to the queue dirty" —
finding 1 contradicts this.
- Undisclosed footprint change: 1 forked JVM → up to numClients
(auto-capped at 4; 4 on any ≥10-core host). Zero mentions of numClients,
concurrency or
threading exist in tika-grpc/README.md or the grpc docs.
- CLIENT_UNAVAILABLE_WITHIN_MS is newly reachable by gRPC clients and
documented nowhere; the proto's status pointer (tika.proto:116) is stale
(org.apache.tika.pipes.PipesResult.STATUS — real type is
...pipes.api.PipesResult.RESULT_STATUS).
- Worth a CHANGES line: this PR is also what makes useSharedServer=true
take effect for tika-grpc for the first time — the old 2-arg constructor
silently forced
per-client mode.
6. [LOW] Two comments describe past code states, against the repo's
terseness rule — TikaGrpcServerImpl.java:124-125 ("...concurrent handler
threads corrupt its
protocol") and the 3-line block at PipesClient.java:230-232.
7. [LOW] Shared-server mode untested at the gRPC layer — both new tests
run per-client only. Mitigated: PipesParser shared-mode concurrency is covered
by
SharedServerModeTest/SharedServerChaosMonkeyTest.
```
> tika-grpc: all parse requests share one single-threaded PipesClient
> -------------------------------------------------------------------
>
> Key: TIKA-4815
> URL: https://issues.apache.org/jira/browse/TIKA-4815
> Project: Tika
> Issue Type: Bug
> Components: tika-pipes
> Affects Versions: 4.0.0
> Reporter: Davide Polato
> Priority: Major
> Labels: grpc, pipes
> Attachments: ConcurrencyLocalDiagnostic.java
>
>
> The gRPC server creates exactly one PipesClient and every parse handler
> thread uses it. PipesClient's own javadoc says it is single-threaded, and
> nothing in TikaGrpcServerImpl synchronizes access to it. The management RPCs
> are not involved - this is about fetchAndParse and its streaming variants,
> which all funnel through the same client.
> Attached is a small repro: one sequential warmup call so the forked worker is
> up and serving, then 4 concurrent fetchAndParse calls at an in-process server
> built without directExecutor (the production server uses a thread pool too).
> Measured on current master:
> DIAG-CONC WARMUP status=PARSE_SUCCESS
> DIAG-CONC OK ... status=UNSPECIFIED_CRASH
> DIAG-CONC OK ... status=FAILED_TO_INITIALIZE
> DIAG-CONC OK ... status=UNSPECIFIED_CRASH
> DIAG-CONC OK ... status=UNSPECIFIED_CRASH
> SUMMARY concurrency=4 answered=4 transportFailed=0 elapsedMs=61358
> Every call got a normal gRPC OK answer - transport-level success masking
> application-level failure. The statuses say the worker died or never came up
> for the request, and four tiny HTML files took a minute. A crawler pointed at
> this records every page as unparseable while the server looks perfectly
> healthy.
> Why no test caught it: TikaGrpcServerTest sends its requests sequentially and
> builds servers with directExecutor(), so nothing ever overlaps.
> Effectively today's server has an undeclared limit of one in-flight parse.
> This is also what blocks cancellation (see TIKA-4795): you can't interrupt
> one request's parse without disturbing the worker everyone else is on.
> Measured on current master (4.0.0-SNAPSHOT). The 3.x server has the same
> shape but I haven't run the probe there.
> tika-pipes already has the right mechanism: PipesParser keeps a queue of
> clients sized by numClients, admission with a timeout, release in finally.
> tika-grpc bypasses it and builds a bare client. Moving the server onto a pool
> looks like most of the fix, with two open questions: what should pool
> exhaustion map to on the wire (I'd lean gRPC RESOURCE_EXHAUSTED -
> infrastructure outcomes as status, parse outcomes in the reply - but that's
> this project's call), and what marks a client as unreusable before pooling -
> PipesClient.process() rethrows InterruptedException without closing the
> connection, and PipesParser's finally re-offers the client unconditionally.
> I can work on this - the repro converts to a regression test directly. Drop
> the attachment into tika-grpc/src/test/java/org/apache/tika/pipes/grpc/ and
> run with -Dtest=ConcurrencyLocalDiagnostic.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
