You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/10/01 00:11:26 UTC
[GitHub] [geode] DonalEvans opened a new pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
DonalEvans opened a new pull request #5582:
URL: https://github.com/apache/geode/pull/5582
Authored-by: Donal Evans <do...@vmware.com>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on a change in pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #5582:
URL: https://github.com/apache/geode/pull/5582#discussion_r504200121
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java
##########
@@ -200,21 +200,28 @@ public Object requestToServer(HostAndPort addr, Object request, int timeout,
out.flush();
if (replyExpected) {
- DataInputStream in = new DataInputStream(sock.getInputStream());
- if (debugVersionMessage != null && logger.isDebugEnabled()) {
- logger.debug(debugVersionMessage);
- }
- in = new VersionedDataInputStream(in, serverVersion);
+ DataInputStream in = null;
try {
Review comment:
Could this have used a `try-with-resources`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-708559861
This pull request **fixes 108 alerts** when merging 5e5e94d439930cfb44929228206e76f89d083d36 into b16709437c8b2f85bfad53a713fde1b9ae1f3903 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-409cec7c9d3b5a147d807c89271ed5acb35390b7)
**fixed alerts:**
* 68 for Potential input resource leak
* 40 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-702474858
This pull request **fixes 57 alerts** when merging 3d95be9de361ebcd27d87602737417b1b3c7dd6e into ce77067a4fea2303f4db33399450a3df80189e29 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-a6bc353a59681e82d3bcc9251f3127bcca266339)
**fixed alerts:**
* 54 for Potential input resource leak
* 3 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-703032244
This pull request **fixes 108 alerts** when merging 1436671264a36a71b4de1b0f3fd34a8cc703cf2d into ae0d6bcdbc41a364b21bc5c55bd982271ad1284b - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-c92d9dcb9fec27c3227855d6181c7475f1bfa7d7)
**fixed alerts:**
* 68 for Potential input resource leak
* 40 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-707325531
This pull request **fixes 108 alerts** when merging f8fb3a4429869e0bba7f4bbd11f1eec5b31e40a2 into e85a1bd450292eb3db098d826734e6ba64151ad0 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-93ef6b048eceea362dae126a136d0d801f26ca7d)
**fixed alerts:**
* 68 for Potential input resource leak
* 40 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5582:
URL: https://github.com/apache/geode/pull/5582#discussion_r504835943
##########
File path: geode-tcp-server/src/main/java/org/apache/geode/distributed/internal/tcpserver/TcpClient.java
##########
@@ -200,21 +200,28 @@ public Object requestToServer(HostAndPort addr, Object request, int timeout,
out.flush();
if (replyExpected) {
- DataInputStream in = new DataInputStream(sock.getInputStream());
- if (debugVersionMessage != null && logger.isDebugEnabled()) {
- logger.debug(debugVersionMessage);
- }
- in = new VersionedDataInputStream(in, serverVersion);
+ DataInputStream in = null;
try {
Review comment:
Good catch, thanks.
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java
##########
@@ -101,26 +101,36 @@ public ResultModel deploy(
for (DistributedMember member : targetMembers) {
List<RemoteInputStream> remoteStreams = new ArrayList<>();
List<String> jarNames = new ArrayList<>();
- for (String jarFullPath : jarFullPaths) {
- remoteStreams
- .add(exporter.export(new SimpleRemoteInputStream(new FileInputStream(jarFullPath))));
- jarNames.add(FilenameUtils.getName(jarFullPath));
- }
-
- // this deploys the jars to all the matching servers
- ResultCollector<?, ?> resultCollector =
- executeFunction(this.deployFunction, new Object[] {jarNames, remoteStreams}, member);
-
- @SuppressWarnings("unchecked")
- final List<List<Object>> resultCollectorResult =
- (List<List<Object>>) resultCollector.getResult();
- results.add(resultCollectorResult.get(0));
+ try {
+ for (String jarFullPath : jarFullPaths) {
+ FileInputStream fileInputStream = null;
+ try {
+ fileInputStream = new FileInputStream(jarFullPath);
+ remoteStreams.add(exporter.export(new SimpleRemoteInputStream(fileInputStream)));
+ jarNames.add(FilenameUtils.getName(jarFullPath));
+ } catch (Exception ex) {
+ if (fileInputStream != null) {
+ fileInputStream.close();
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-703003162
This pull request **fixes 111 alerts** when merging 139a7570d85022ec58dbce02606caeb802224f3c into ae0d6bcdbc41a364b21bc5c55bd982271ad1284b - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-b2a44eb6414d6d56d08eb3e667a62544c7a6a645)
**fixed alerts:**
* 69 for Potential input resource leak
* 42 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-702979271
This pull request **fixes 121 alerts** when merging 1f8fab8534cd67fd97f11e241e56728c3d08be54 into 6d58009eb7b391dfbdbe8c673af596ab4da41096 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-def019f3a18ad157106351278a9c1d724f957bd8)
**fixed alerts:**
* 70 for Potential input resource leak
* 51 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] removed a comment on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] removed a comment on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-703032244
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-702293516
This pull request **fixes 59 alerts** when merging db9a023aa2fae3b9760de42d0fe45b2398665d0b into a099fa361b6820343f47dac6a44bc63cbb754396 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-2abc24aa50d9bc0690a903e43069ce6646335c99)
**fixed alerts:**
* 55 for Potential input resource leak
* 4 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] upthewaterspout commented on a change in pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
upthewaterspout commented on a change in pull request #5582:
URL: https://github.com/apache/geode/pull/5582#discussion_r504198531
##########
File path: geode-gfsh/src/main/java/org/apache/geode/management/internal/cli/commands/DeployCommand.java
##########
@@ -101,26 +101,36 @@ public ResultModel deploy(
for (DistributedMember member : targetMembers) {
List<RemoteInputStream> remoteStreams = new ArrayList<>();
List<String> jarNames = new ArrayList<>();
- for (String jarFullPath : jarFullPaths) {
- remoteStreams
- .add(exporter.export(new SimpleRemoteInputStream(new FileInputStream(jarFullPath))));
- jarNames.add(FilenameUtils.getName(jarFullPath));
- }
-
- // this deploys the jars to all the matching servers
- ResultCollector<?, ?> resultCollector =
- executeFunction(this.deployFunction, new Object[] {jarNames, remoteStreams}, member);
-
- @SuppressWarnings("unchecked")
- final List<List<Object>> resultCollectorResult =
- (List<List<Object>>) resultCollector.getResult();
- results.add(resultCollectorResult.get(0));
+ try {
+ for (String jarFullPath : jarFullPaths) {
+ FileInputStream fileInputStream = null;
+ try {
+ fileInputStream = new FileInputStream(jarFullPath);
+ remoteStreams.add(exporter.export(new SimpleRemoteInputStream(fileInputStream)));
+ jarNames.add(FilenameUtils.getName(jarFullPath));
+ } catch (Exception ex) {
+ if (fileInputStream != null) {
+ fileInputStream.close();
Review comment:
Does this need to swallow IOExceptions?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-701723234
This pull request **fixes 68 alerts** when merging ced0729db409cd5bb4b445355ee1601e4ad30528 into 2a993a5b567a39c85b5116df68bf1dfa598edbf3 - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-e0b8243796474e12af65fa3858c7704f41fea295)
**fixed alerts:**
* 60 for Potential input resource leak
* 8 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans merged pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #5582:
URL: https://github.com/apache/geode/pull/5582
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5582: GEODE-7864: Fix potential resource leak LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5582:
URL: https://github.com/apache/geode/pull/5582#issuecomment-703372830
This pull request **fixes 108 alerts** when merging 12ba31be7ea11d260fdc1d614e1fc31402f19228 into c91e91592bffadefffd221a0946ee8f9e229190b - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-c80fbcda428c9ed51418a09aa271744c9e4c9363)
**fixed alerts:**
* 68 for Potential input resource leak
* 40 for Potential output resource leak
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org