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