-
Notifications
You must be signed in to change notification settings - Fork 4.4k
fix: close InputStreams after StreamUtils.copyToString #41516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release
Are you sure you want to change the base?
fix: close InputStreams after StreamUtils.copyToString #41516
Conversation
…source leaks StreamUtils.copyToString() does not close the InputStream after reading, which can lead to resource leaks and file descriptor exhaustion. This change wraps all StreamUtils.copyToString() calls with try-with-resources to ensure InputStreams are properly closed. Fixes appsmithorg#41495
WalkthroughReplaced unsafe StreamUtils.copyToString(...) usages with try-with-resources InputStream blocks across production and test files to ensure streams are closed after reading resources; no public APIs or functional behavior changed. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@sharat87 Hi, just a gentle ping to check if this PR is ready to be merged. |
subrata71
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing these resource leaks. There may other unclosed InputStream in PluginServiceCEImpl.java . Could you please check and apply the same try-with-resources pattern if needed.
InputStream obtained from getInputStream() or getResourceAsStream() must be explicitly closed to prevent resource leaks and file descriptor exhaustion. This change wraps all InputStream usages with try-with-resources to ensure they are properly closed. Fixes appsmithorg#41495
…o fix/resource-leak-streamutils
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java`:
- Around line 821-831: The code calls
classLoader.getResourceAsStream(gitServiceConfig.getReadmeTemplatePath())
without checking for null which can cause an NPE at IOUtils.copy; update the
FileUtilsCEImpl logic to check the InputStream for null (similar to the pattern
used in BashService.java) and handle the missing resource (throw a clear
exception or use a fallback) before calling IOUtils.copy; also remove the
redundant .toString() when replacing EDIT_MODE_URL_TEMPLATE and
VIEW_MODE_URL_TEMPLATE on the already-String variable data to simplify the
replacement chain.
app/server/appsmith-git/src/main/java/com/appsmith/git/files/FileUtilsCEImpl.java
Show resolved
Hide resolved
Ensure an exception is thrown with a clear message if the README template is not found, preventing potential null pointer issues.
|
@aejeong-context Some quality checks are failing specially spotless check. Could you please fix that by running the following command from /app/server directory?
|
Fixes #41495
What was changed
Why
StreamUtils.copyToString() does not close the provided InputStream.
Leaving these streams open can lead to resource leaks and file descriptor exhaustion.
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.