CAMEL-24120: camel-smb - Fix double download and stream download file handle leak#24788
Conversation
… handle leak SmbFile.getBody() bypassed the body stored by retrieveFile and always re-downloaded the file from the server. Fixed to return the binding body first, falling back to lazy fetch only when no body was stored. For streamDownload=true, the smbj File handle was leaked because FileInputStream.close() does not close the underlying File. Introduced SmbFileClosingInputStream wrapper whose close() also closes the smbj File handle. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Claus Ibsen <claus.ibsen@gmail.com>
|
🌟 Thank you for your contribution to the Apache Camel project! 🌟 🐫 Apache Camel Committers, please review the following items:
|
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Well-designed fix for three interrelated bugs in the SMB stream download path. The issues have been present since streamDownload was added in CAMEL-21352, but bug #3 was masked by bug #1 (double download always re-opened a fresh handle).
Bug 1 — Double download (SmbFile.getBody()):
The override always called operations.getBodyAsInputStream() / operations.getBody(), re-downloading the file even though retrieveFileToStreamInBody already stored the body via the binding. Fix correctly checks super.getBody() first (which delegates to getBinding().getBody(this) on GenericFile), falling back to lazy fetch only for pollEnrich or paths that skip retrieveFile. ✅
Bug 2 — File handle leak (SmbFileClosingInputStream):
smbj's FileInputStream.close() does NOT close the underlying File handle — confirmed by the library's design. The new SmbFileClosingInputStream extends FilterInputStream and closes the smbj File in finally on close(). Error handling is correct: if getInputStream() throws during setup, the catch(Exception) block closes the File handle via IOHelper.close(shareFile). ✅
Bug 3 — Dead-on-arrival stream (retrieveFileToStreamInBody):
The old code wrapped the File handle in try-with-resources even for the stream download path, so the handle was closed before the stream was ever read. Fix correctly splits the stream/non-stream paths: non-stream keeps try-with-resources (reads all bytes); stream keeps the File open, wrapped in SmbFileClosingInputStream. ✅
Design observations:
- The NOSONAR comment on
getBodyAsInputStream()is correctly removed — theFilehandle is now properly managed viaSmbFileClosingInputStream - Both
retrieveFileToStreamInBodyandgetBodyAsInputStreamconsistently use the same wrapper pattern SmbFileClosingInputStreamis correctly package-private — internal implementation detail
Static analysis: ast-grep ran on all modified/new files — no findings.
Checklist:
- ✅ Three bugs fixed with minimal, focused changes
- ✅ Resource management is correct (handle closed on error, on stream close, and on non-stream path)
- ✅ No new dependencies
- ✅ Commit convention followed
- ✅ CI pending (builds running)
Reviewed with Claude Code (claude-code/1.0) on behalf of gnodet. This review was generated by an AI agent and may contain inaccuracies; please verify all suggestions before applying.
|
🧪 CI tested the following changed modules:
🔬 Scalpel shadow comparison — Scalpel: 9 tested, 29 compile-only — current: 9 all testedMaveniverse Scalpel detected 38 affected modules (current approach: 9).
|
2 participants
Summary
SmbFile.getBody()was bypassing the body already stored byretrieveFileand always re-downloading the file from the server. Now returns the binding body first, falling back to lazy fetch only when no body was stored (e.g.,pollEnrich).streamDownload=true, the smbjFilehandle was leaked becauseFileInputStream.close()does not close the underlyingFile. IntroducedSmbFileClosingInputStreamwrapper whoseclose()also closes the smbjFilehandle.retrieveFileToStreamInBodywas wrapping the smbjFilein try-with-resources even for the stream download path, closing the handle before the stream was ever read. This was masked by the double download bug.Test plan
streamDownload=truedoes not leak SMB file handles after stream is consumedClaude Code on behalf of davsclaus
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com