-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[ZEPPELIN-6268] Fix resource leaks and add null checks for getResourceAsStream #5010
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: master
Are you sure you want to change the base?
Conversation
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.
As you can see, I am a big fan of try-with-resources.
This is a JDK8 feature and should be used because it saves a lot of boilerplate code.
fis.read(read); | ||
fis.close(); | ||
assertTrue(BufferUtils.equalIncreasingByteArray(size, read)); | ||
try { |
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.
Please use try-with-resources. This is more elegant.
https://docs.oracle.com/javase/tutorial/essential/exceptions/tryResourceClose.html
String dstFileName) throws IOException { | ||
FileOutputStream out = null; | ||
InputStream in = null; | ||
try { |
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.
Please use try-with-resources, also here
} | ||
|
||
private void initPythonInterpreter(String gatewayHost, int gatewayPort) throws IOException { | ||
InputStream input = |
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.
Here, closing the input stream is missing; this can be done directly with try-with-resource.
throw new IOException("Fail to setup JVMGateway\n" + response.getOutput()); | ||
} | ||
|
||
input = |
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.
Here, closing the input stream is missing; this can be done directly with try-with-resource.
} | ||
|
||
if (additionalPythonInitFile != null) { | ||
input = getClass().getClassLoader().getResourceAsStream(additionalPythonInitFile); |
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.
Here, closing the input stream is missing; this can be done directly with try-with-resource.
*/ | ||
protected void initIRKernel() throws IOException, InterpreterException { | ||
String timeout = getProperty("spark.r.backendConnectionTimeout", "6000"); | ||
InputStream input = |
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.
Same please use try-with-resource do close the stream.
File scriptFile = File.createTempFile("zeppelin_sparkr-", ".R"); | ||
FileOutputStream out = null; | ||
InputStream in = null; | ||
try { |
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.
Same use try-with-resource for the input and FileOutputStream
What is this PR for?
This PR fixes resource management issues throughout the Zeppelin codebase, specifically addressing resource leaks and potential NullPointerExceptions.
What type of PR is it?
Bug Fix / Improvement
What is the Jira issue?
(Please create a JIRA issue if needed)
How should this be tested?
Screenshots (if appropriate)
N/A
Questions:
Changes Made:
1. AlluxioInterpreterTest.java
2. Resource Loading Null Checks
Added null checks for
getResourceAsStream()
calls in:Benefits:
Testing Notes:
While I couldn't run all tests due to environment setup issues, all files compile successfully and the changes are minimal and safe - they only add defensive programming practices without changing any business logic.