[
https://issues.apache.org/jira/browse/CAMEL-24120?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18096799#comment-18096799
]
Claus Ibsen commented on CAMEL-24120:
-------------------------------------
Fixed via https://github.com/apache/camel/pull/24788
> camel-smb: consumer downloads every file twice; streamDownload=true leaks a
> remote file handle per file (SmbFile.getBody bypasses the retrieved body)
> -----------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CAMEL-24120
> URL: https://issues.apache.org/jira/browse/CAMEL-24120
> Project: Camel
> Issue Type: Bug
> Components: camel-smb
> Affects Versions: 4.21.0
> Reporter: Federico Mariani
> Assignee: Claus Ibsen
> Priority: Major
> Labels: code-review
> Fix For: 4.22.0
>
>
> h3. Problem 1 - every consumed file is downloaded twice
> {{SmbConsumer}} runs the standard retrieve pipeline:
> {{GenericFileConsumer.processExchange}} -> {{SmbOperations.retrieveFile}} ->
> {{retrieveFileToStreamInBody}} reads the *entire file* into a {{byte[]}} and
> stores it on the {{GenericFile}} via {{target.setBody(body)}}
> (SmbOperations.java:301-330). But the message body is resolved lazily through
> {{GenericFileMessage.createBody()}} -> {{file.getBody()}}, and {{SmbFile}}
> overrides {{getBody()}} (SmbFile.java:89-99) to bypass the stored body
> entirely:
> {code:java}
> @Override
> public Object getBody() {
> if (!download) {
> return null;
> }
> if (streamDownload) {
> return operations.getBodyAsInputStream(exchange,
> this.getAbsoluteFilePath());
> } else {
> return operations.getBody(this.getAbsoluteFilePath()); // fresh
> full download
> }
> }
> {code}
> The bytes fetched by {{retrieveFile}} are never used - the first body access
> issues a *second full download*. Consequences with the default configuration:
> * 2x network transfer and 2x server reads for every consumed file.
> * {{localWorkDirectory}} is completely defeated:
> {{retrieveFileToFileInLocalWorkDirectory}} spools the file to local disk
> (whose whole point is keeping big files out of memory), then {{getBody()}}
> re-downloads the whole file *into memory* anyway; the local file is never
> read.
> * The bytes the route sees come from a later read than the one the
> {{changed}} read lock validated.
> h3. Problem 2 - streamDownload=true leaks one remote file handle per file
> smbj 0.14.0's {{FileInputStream.close()}} only sets {{isClosed = true}} and
> nulls its fields - verified in the bytecode, it does *not* close the
> underlying {{File}} (the remote SMB handle), and neither does reading to EOF.
> {{SmbOperations.getBodyAsInputStream}} (SmbOperations.java:626-641)
> deliberately keeps the {{File}} open ({{// NOSONAR}}) but retains no
> reference to it, and {{releaseRetrievedFileResources}}
> (SmbOperations.java:416-425) only closes the *stream* from the
> {{CamelSmbFileInputStream}} header. Net effect: every file consumed with
> {{streamDownload=true}} leaks one open SMB file handle on the session until
> the consumer disconnects or stops ({{disconnect=false}} is the default, so
> for a long-running consumer that is effectively forever). Handles grow
> without bound; consumed-and-deleted files stay in delete-pending state
> (removal deferred until the last - leaked - handle closes); other clients can
> hit sharing violations on files Camel has already finished with.
> Additionally, the stream created during the retrieve step is dead on arrival:
> {{retrieveFileToStreamInBody}} opens the smbj {{File}} in try-with-resources,
> stores {{getInputStream()}} as body and in the header, then closes the
> {{File}} when the block exits - any read of that stream fails with
> STATUS_FILE_CLOSED. This is currently masked only by Problem 1 (the body
> access re-opens the file and overwrites the header).
> h3. History
> The lazy {{getBody()}} fetch dates from the CAMEL-21277 WrappedFile rework
> (4.9.0), when the consumer did not yet run the retrieve pipeline. CAMEL-21352
> then added the {{download}}/{{streamDownload}}/{{localWorkDirectory}}
> retrieve machinery on top without removing the lazy override, producing both
> the duplicate download and the second (leaked) open.
> h3. Suggested fix
> Both defects have the same root - body handling split across {{retrieveFile}}
> and {{getBody()}} - and should be fixed together:
> * {{getBody()}} should return the body captured by {{retrieveFile}} (via the
> binding, like FTP's {{RemoteFile}}) and only fall back to a lazy fetch when
> there is none (e.g. {{poll}}/{{pollEnrich}} via
> {{GenericFilePollingConsumer}}).
> * The streamDownload stream must be wrapped in a {{FilterInputStream}} whose
> {{close()}} also closes the smbj {{File}}, so
> {{releaseRetrievedFileResources}}/route completion actually releases the
> remote handle; and the retrieve step must not close the handle backing a
> stream it hands out.
> ----
> _This issue was researched and filed by Claude Code on behalf of [~fmariani]
> (GitHub: Croway), as part of a deep code review of camel-smb. Verified
> against the generic-file framework call chain and the smbj 0.14.0 bytecode;
> observing the duplicate transfer / leaked handle requires server-side
> instrumentation, so no JUnit reproducer is attached for this one._
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
