[
https://issues.apache.org/jira/browse/TIKA-4991?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18130892#comment-18130892
]
ASF GitHub Bot commented on TIKA-4991:
--------------------------------------
Copilot commented on code in PR #3404:
URL: https://github.com/apache/tika/pull/3404#discussion_r4359659771
##########
tika-server/tika-server-core/src/main/java/org/apache/tika/server/core/MaxRequestSizeFilter.java:
##########
@@ -84,19 +79,11 @@ public void filter(ContainerRequestContext requestContext) {
requestContext.abortWith(tooLarge());
return;
}
- long maxReadNanos = maxRequestReadMillis < 0 ? -1 :
TimeUnit.MILLISECONDS.toNanos(maxRequestReadMillis);
RequestBodyBudget.Reservation reservation =
budget == null ? null :
budget.open(JAXRSUtils.getCurrentMessage());
// Entity bytes: after any Content-Encoding is undone.
requestContext.setEntityStream(new
BoundedInputStream(requestContext.getEntityStream(),
- maxRequestSizeBytes, startNanos(), maxReadNanos,
maxRequestReadMillis, reservation));
- }
-
- private static long startNanos() {
- Message message = JAXRSUtils.getCurrentMessage();
- Object start = message == null || message.getExchange() == null
- ? null :
message.getExchange().get(RequestReadDeadlineInterceptor.START_NANOS);
- return start instanceof Long l ? l : System.nanoTime();
+ maxRequestSizeBytes, -1, -1, -1, reservation));
Review Comment:
Passing `-1` sentinel values through `BoundedInputStream` makes the call
site hard to understand and keeps now-disabled deadline code paths around.
Consider adding an overload (or named constants) for the size-only case (and/or
removing the unused deadline fields/logic if they’re no longer used anywhere)
so it’s clear this filter only enforces size/budget here.
##########
tika-server/tika-server-standard/src/test/java/org/apache/tika/server/standard/MaxRequestSizeIntegrationTest.java:
##########
@@ -215,6 +215,28 @@ public void testOptionsBodyNotSpooled() throws Exception {
}
}
+ /**
+ * CXF reads the multipart closing boundary only after the resource
returns; a parse that
+ * outlasts the read deadline must not turn into a 408 there.
+ */
+ @Test
+ public void testParseOutlastingReadDeadlineMultipart() throws Exception {
+ String mock = "<mock><write element=\"p\">slow parse done</write><hang
millis=\""
+ + 2 * MAX_READ_MILLIS + "\" heavy=\"false\"
pulse_millis=\"100\"/></mock>";
Review Comment:
This test’s pass/fail depends on timing (parse duration vs configured read
deadline), which can be flaky under load/slow CI. To reduce flakiness, consider
minimizing the margin while still exceeding the deadline deterministically
(e.g., `MAX_READ_MILLIS + fixedBuffer`), and/or making the test assert the
server-side behavior more directly (so it does not rely on
longer-than-necessary sleeps).
##########
tika-server/tika-server-core/src/main/java/org/apache/tika/server/core/MaxRequestSizeFilter.java:
##########
@@ -190,12 +179,19 @@ public long skip(long n) throws IOException {
return skipped;
}
- private void checkDeadline() {
- if (maxReadNanos >= 0 && System.nanoTime() - startNanos >
maxReadNanos) {
+ private void checkDeadline() throws IOException {
+ if (maxReadNanos >= 0 && System.nanoTime() - startNanos >
maxReadNanos && wouldBlock()) {
throw new RequestTimeoutException(TOO_SLOW_MESSAGE + " (" +
maxReadMillis + " ms)");
}
}
+ private boolean wouldBlock() throws IOException {
+ if (in instanceof ServletInputStream servletIn &&
servletIn.isFinished()) {
+ return false;
+ }
Review Comment:
`InputStream.available()` is not a reliable way to determine whether a read
would block (it may return 0 even when a read would not block, depending on the
stream implementation/wrappers). Since this logic is specifically about servlet
request streams, prefer `ServletInputStream.isReady()` (and `isFinished()`)
when `in` is a `ServletInputStream`; only fall back to `available()` (or keep
the old behavior) for non-servlet streams.
> Improve limits in tika-server
> -----------------------------
>
> Key: TIKA-4991
> URL: https://issues.apache.org/jira/browse/TIKA-4991
> Project: Tika
> Issue Type: Task
> Reporter: Tim Allison
> Priority: Minor
>
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
