You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by "ASF GitHub Bot (Jira)" <ji...@apache.org> on 2021/08/05 09:44:00 UTC
[jira] [Commented] (QPID-8550) [Broker-J] HP Fortify: Unreleased
streams
[ https://issues.apache.org/jira/browse/QPID-8550?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17393757#comment-17393757 ]
ASF GitHub Bot commented on QPID-8550:
--------------------------------------
alex-rufous commented on a change in pull request #103:
URL: https://github.com/apache/qpid-broker-j/pull/103#discussion_r683282742
##########
File path: broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java
##########
@@ -156,49 +156,52 @@ protected boolean load(final ConfiguredObjectRecord... initialRecords)
boolean updated = false;
Collection<ConfiguredObjectRecord> records = Collections.emptyList();
- ConfiguredObjectRecordConverter configuredObjectRecordConverter =
+ final ConfiguredObjectRecordConverter configuredObjectRecordConverter =
new ConfiguredObjectRecordConverter(_parent.getModel());
- records = configuredObjectRecordConverter.readFromJson(_rootClass, _parent, new FileReader(configFile));
-
- if(_rootClass == null)
+ try (FileReader configFileReader = new FileReader(configFile))
Review comment:
This change is unnecessary. The `configFileReader` is closed in `ObjectMapper`. If you really need to make sure that `configFileReader` is closed in the code where it is created, you can simply add `try-with-resource` block around the code reading from json
```
try (FileReader configFileReader = new FileReader(configFile))
{
records = configuredObjectRecordConverter.readFromJson(_rootClass, _parent, configFileReader);
}
```
##########
File path: broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet/rest/RestServlet.java
##########
@@ -437,7 +437,10 @@ public String getRequestURL()
{
if ("data".equals(part.getName()) && "application/json".equals(part.getContentType()))
{
- items = mapper.readValue(part.getInputStream(), LinkedHashMap.class);
+ try (InputStream inputStream = part.getInputStream())
Review comment:
The change is unnecessary as methods `ObjectMapper#readValue ` close the input stream/reader..
##########
File path: perftests/src/main/java/org/apache/qpid/disttest/controller/config/ConfigReader.java
##########
@@ -37,10 +37,10 @@
public Config getConfigFromFile(String fileName) throws IOException
{
- Reader reader = getConfigReader(fileName);
-
- Config config = readConfig(reader);
- return config;
+ try (Reader reader = getConfigReader(fileName))
Review comment:
The reader is closed in `ObjectMapper#read...`. Thus, the change is unnecessary...
##########
File path: broker-core/src/main/java/org/apache/qpid/server/util/FileUtils.java
##########
@@ -205,8 +205,10 @@ public static void copy(File src, File dst)
*/
public static void copyCheckedEx(File src, File dst) throws IOException
{
- InputStream in = new FileInputStream(src);
- copy(in, dst);
+ try (InputStream in = new FileInputStream(src))
Review comment:
The `InputStream` is closed in `copy`. Arguably the method `copy` should indicate that it closes the input stream. It make more sense to rename it into `copyInputStreamIntoFileAndClose`. Though, I suppose for a better code relaibility the 'try-with-resource` would be good here.
##########
File path: broker-core/src/main/java/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java
##########
@@ -430,40 +430,42 @@ public PreferenceStore createPreferenceStore()
protected final ConfiguredObjectRecord[] getInitialRecords() throws IOException
{
- ConfiguredObjectRecordConverter converter = new ConfiguredObjectRecordConverter(getModel());
+ final ConfiguredObjectRecordConverter converter = new ConfiguredObjectRecordConverter(getModel());
- Collection<ConfiguredObjectRecord> records =
- new ArrayList<>(converter.readFromJson(VirtualHost.class,this,getInitialConfigReader()));
-
- if(!records.isEmpty())
+ try (final Reader initialConfigReader = getInitialConfigReader())
Review comment:
Try-with-resource block is unnecessary here as reading code is closing the reader. If you really have to change to `try-with-resource`, it would be preferable to keep the `try` block small and only use it for reading the records. IMHO, that makes code more readable
```
final Collection<ConfiguredObjectRecord> records;
try (final Reader initialConfigReader = getInitialConfigReader())
{
records = new ArrayList<>(converter.readFromJson(VirtualHost.class, this, initialConfigReader));
}
```
--
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: dev-unsubscribe@qpid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
> [Broker-J] HP Fortify: Unreleased streams
> -----------------------------------------
>
> Key: QPID-8550
> URL: https://issues.apache.org/jira/browse/QPID-8550
> Project: Qpid
> Issue Type: Improvement
> Components: Broker-J
> Affects Versions: qpid-java-broker-8.0.5
> Reporter: Daniil Kirilyuk
> Priority: Minor
>
> HP Fortify complains about several places where streams potentially do not get released properly:
> broker-core/src/main/java/org/apache/qpid/server/store/JsonFileConfigStore.java
> The function load() in JsonFileConfigStore.java sometimes fails to release a system resource allocated by FileReader() on line 162.
> broker-core/src/main/java/org/org/apache/qpid/server/virtualhostnode/AbstractVirtualHostNode.java
> The function getInitialRecords() in AbstractVirtualHostNode.java sometimes fails to release a system resource allocated by ,[object Object], on line 435.
> broker-plugins/management-http/src/main/java/org/apache/qpid/server/management/plugin/servlet.rest/RestServlet.java
> The function parse() in RestServlet.java sometimes fails to release a system resource allocated by getInputStream() on line 440.
> tools/src/main/java/org/apache/qpid/tools/RestStressTestClient.java
> The function put() in RestStressTestClient.java sometimes fails to release a system resource allocated by getOutputStream() on line 318.
> perftests/src/main/java/org/apache/qpid/disttest/controller/config/ConfigReader.java
> The function getConfigFromFile() in ConfigReader.java sometimes fails to release a system resource allocated by getConfigReader() on line 40.
> perftests/src/main/java/org/apache/qpid/disttest/controller/config/JavaScriptConfigEvaluator.java
> The function evaluateJavaScript() in JavaScriptConfigEvaluator.java sometimes fails to release a system resource allocated by InputStreamReader() on line 70.
> The function evaluateJavaScript() in JavaScriptConfigEvaluator.java sometimes fails to release a system resource allocated by InputStreamReader() on line 71.
>
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@qpid.apache.org
For additional commands, e-mail: dev-help@qpid.apache.org