Copilot commented on code in PR #3211:
URL: https://github.com/apache/tika/pull/3211#discussion_r4054082052
##########
tika-parsers/tika-parsers-ml/tika-vlm/src/main/java/org/apache/tika/parser/vlm/AbstractVLMParser.java:
##########
@@ -430,8 +434,12 @@ public void setTimeoutMillis(long timeoutMillis) {
}
/** Retries of a 429/502/503/504 answer from the service; 0 fails at once.
*/
- public void setMaxRetries(int maxRetries) {
+ public synchronized void setMaxRetries(int maxRetries) throws IOException {
defaultConfig.setMaxRetries(maxRetries);
+ if (httpClient != null) {
+ httpClient.close();
+ httpClient = null;
+ }
}
Review Comment:
Adding checked `IOException` to this public setter is source-incompatible
for existing callers and is unnecessary because `TikaHttpClient.close()` is
non-throwing. Keep the setter signature unchanged and handle any future close
failure internally.
##########
tika-core/src/main/java/org/apache/tika/io/ReopenableSource.java:
##########
@@ -276,13 +278,29 @@ private void maybeReleaseRetained() {
* grows, reserving, on what is actually read. Does not disturb this
source's read
* position.
*/
+ /** A declared length the budget cannot cover is not attempted; unknown is
attempted. */
+ private boolean mayFitInMemory() {
+ return length <= IN_MEMORY_FLOOR || (budget != null
+ && budget.getMaxBytes() - budget.getReservedBytes() >= length
- IN_MEMORY_FLOOR);
+ }
+
private boolean tryBufferInMemory() throws IOException {
if (length > MAX_ARRAY_SIZE || (length > IN_MEMORY_FLOOR && budget ==
null)) {
return false;
}
long reservedHere = 0;
// Reservation invariant: reservedHere == max(0, data.length -
IN_MEMORY_FLOOR)
- byte[] data = new byte[(int) Math.max(8192, Math.min(length,
IN_MEMORY_FLOOR))];
+ int initial = (int) Math.max(8192, Math.min(length, IN_MEMORY_FLOOR));
+ if (length > IN_MEMORY_FLOOR && budget != null) {
+ // one reservation for a declared length the budget covers; a
refusal is not a
+ // verdict, since the length may lie, so the ladder from the floor
still runs
+ long delta = length - IN_MEMORY_FLOOR;
+ if (budget.tryReserve(delta) == delta) {
+ reservedHere = delta;
+ initial = (int) length;
+ }
Review Comment:
`length` comes from caller metadata (`Content-Length`) and can be
arbitrarily large, but this path reserves the full declared delta and
immediately allocates an array of that size before reading the opener. A
request claiming a multi-gigabyte length can exhaust a worker even when its
actual stream is small; start at the floor and let the existing growth ladder
reserve only bytes that are actually read.
##########
tika-parsers/tika-http-jdk/src/main/java/org/apache/tika/http/TikaHttpClient.java:
##########
@@ -98,6 +98,10 @@ private TikaHttpClient(HttpClient httpClient,
ExecutorService executor,
*
* @param connectTimeoutSeconds TCP connection timeout in seconds
*/
+ public int getMaxRetries() {
+ return maxRetries;
+ }
Review Comment:
The existing factory Javadoc is now attached to `getMaxRetries()` (including
an irrelevant `connectTimeoutSeconds` parameter), while the one-argument
`build` factory is left without that documentation. Move this getter after the
factory overloads or add its own Javadoc without separating the factory comment
from `build`.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-audiovideo-module/src/main/java/org/apache/tika/detect/ogg/OggDetector.java:
##########
@@ -76,7 +76,7 @@ public MediaType detect(TikaInputStream tis, Metadata
metadata, ParseContext par
// We could potentially need to go a long way through the
// file in order to figure out what it is
- tis.mark((int)tis.getLength() + 1);
+ tis.mark(tis.hasLength() ? (int) Math.min(tis.getLength() + 1,
Integer.MAX_VALUE) : Integer.MAX_VALUE);
Review Comment:
For a raw stream with unknown length, this reaches `CachingSource`'s
passthrough `BufferedInputStream` and requests a `Integer.MAX_VALUE` mark. As
`OggFile` scans a large input, that buffer can grow toward 2 GiB; the previous
unknown-length path materialized the stream into file-backed mode before
marking. Use bounded replay/`enableRewind` instead of an unbounded mark, and
avoid the `getLength() + 1` overflow when a declared length is `Long.MAX_VALUE`.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-pkg-module/src/main/java/org/apache/tika/parser/pkg/SevenZParser.java:
##########
@@ -115,6 +115,14 @@ public void parse(TikaInputStream tis, ContentHandler
handler, Metadata metadata
SevenZFile sevenZFile;
// SevenZFile.close() closes the channel it was built on
+ metadata.set(HttpHeaders.CONTENT_TYPE, SEVENZ.toString());
+
+ EmbeddedDocumentExtractor extractor =
+ EmbeddedDocumentUtil.getEmbeddedDocumentExtractor(context);
+
+ XHTMLContentHandler xhtml = new XHTMLContentHandler(handler, metadata,
context);
+ xhtml.startDocument();
Review Comment:
`startDocument()` is now fired before channel/archive construction. If
`getSeekableByteChannel()` or `SevenZFile.Builder.get()` rejects a corrupt,
encrypted, or over-limit archive, the handler receives `startDocument` without
the `endDocument` that is only emitted by the later `finally` block. Delay the
SAX start until after the builder succeeds, or cover setup and the SAX
lifecycle with one cleanup block.
##########
tika-parsers/tika-parsers-standard/tika-parsers-standard-modules/tika-parser-image-module/src/main/java/org/apache/tika/parser/image/ImageParser.java:
##########
@@ -170,8 +171,9 @@ void extractMetadata(InputStream stream, ContentHandler
contentHandler, Metadata
if (iterator.hasNext()) {
ImageReader reader = iterator.next();
try {
- try (ImageInputStream imageStream = ImageIO
-
.createImageInputStream(CloseShieldInputStream.wrap(stream))) {
+ // memory-cached: ImageIO's default cache writes every
byte read to a temp file
+ try (ImageInputStream imageStream =
+ new
MemoryCacheImageInputStream(CloseShieldInputStream.wrap(stream))) {
Review Comment:
`MemoryCacheImageInputStream` retains every byte requested by the
`ImageReader` on the Java heap. For an untrusted image stream with a large
payload or trailing data, this changes the previous disk-backed ImageIO cache
into an unbounded per-parse allocation and can exhaust the server; keep a
disk-backed cache or enforce a bounded cache instead.
--
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]
