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.
```
--
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]
