Opened 5 months ago

Closed 4 weeks ago

#30544 closed defect (fixed)

Using try-with-resources or close resource

Reported by: fava Owned by: metrics-team
Priority: Medium Milestone:
Component: Metrics/Library Version:
Severity: Major Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Sonarqube identifies a list of case as:
Use try-with-resources or close this ...

Please find in attachment a detailed report for this issues type.

It is required to check or use try-with-resources statementa change in source code.

Child Tickets

Attachments (6)

try-with-resources.csv (3.7 KB) - added by fava 5 months ago.
Sonarqube try-with-resources
patch-fix-resource-leak.zip (3.3 KB) - added by fava 4 months ago.
30544-resource-leak-v2.zip (3.1 KB) - added by fava 8 weeks ago.
Version 2 patch
0001-Fix-leaking-resource-in-TorperfResultImpl.java.patch (3.0 KB) - added by fava 7 weeks ago.
Fix leaking resource TorpertResultImpl
actions-trac-30544.png (12.1 KB) - added by fava 7 weeks ago.
Actions trac
30544-resource-leak-v4.tar.gz (12.5 KB) - added by fava 6 weeks ago.

Download all attachments as: .zip

Change History (17)

Changed 5 months ago by fava

Attachment: try-with-resources.csv added

Sonarqube try-with-resources

comment:1 Changed 5 months ago by karsten

Would you be able to submit a patch that fixes one instance of this issue? It could be a GitHub branch or a patch created with git format-patch.

Regarding the fix, I'd say that using try-with-resources is better than explicitly closing the resource. But let's discuss this more once there's a patch.

Thanks!

comment:2 Changed 5 months ago by irl

GitHub mirror is here if you want to fork it: https://github.com/torproject/metrics-lib

comment:3 in reply to:  2 Changed 5 months ago by fava

Replying to irl:

GitHub mirror is here if you want to fork it: https://github.com/torproject/metrics-lib

I just created a fork and pushed a specific branch for this ticket https://github.com/f-a-v-a/metrics-lib/tree/30544-try-with-resources.

At the moment this branch contains my temporary work and I did not complete the task.

I will inform you when I finish the development.

Changed 4 months ago by fava

Attachment: patch-fix-resource-leak.zip added

comment:4 in reply to:  1 Changed 4 months ago by fava

Replying to karsten:

Would you be able to submit a patch that fixes one instance of this issue? It could be a GitHub branch or a patch created with git format-patch.

Replying to irl:

GitHub mirror is here if you want to fork it: ​https://github.com/torproject/metrics-lib

Please find in attachment my patch files patch-fix-resource-leak.zip
There is also a specific github branch https://github.com/f-a-v-a/metrics-lib/tree/30544-try-with-resources .

Please let me know your code review,
Best Regards

Last edited 4 months ago by fava (previous) (diff)

comment:5 Changed 3 months ago by karsten

Status: newneeds_revision

This took much longer than it should have, sorry for that. Trying to get faster with future reviews!

Here's what I found:

  • There are some minor whitespace issues. Please run ant checks before submitting a patch.
  • Those TODO comments look reasonable, but let's maybe try to just fix them now, each of them in a separate commit. Otherwise we'll just collect more things to do in the sources.
  • Our commit messages typically start with a roughly 50 chars long summary line, followed by two newlines, followed by text wrapped to 70 chars. Can you try to rephrase your commit messages accordingly?
  • Can you rebase your edited commits to master?
  • When you post a patch or branch, be sure to change status to needs_review. To be clear, this is not the main reason for us not reviewing sooner, but it could be a possible reason for future delays.

Thanks again!

Changed 8 weeks ago by fava

Attachment: 30544-resource-leak-v2.zip added

Version 2 patch

comment:6 in reply to:  5 Changed 8 weeks ago by fava

Hi karsten

Replying to karsten:

  • There are some minor whitespace issues. Please run ant checks before submitting a patch.

I created a new version of path starting from updated master, fix ant checks whitespace and comment rewrited with your advice 30544-resource-leak-v2.zip​

I also update my remote branch https://github.com/f-a-v-a/metrics-lib/tree/30544-try-with-resources .

  • Those TODO comments look reasonable, but let's maybe try to just fix them now, each of them in a separate commit. Otherwise we'll just collect more things to do in the sources.

I prefer to create a new trac tasks for TODO comment after this patch will be merged into master branch. This help me to focus on one activity at time and split different activities

  • Our commit messages typically start with a roughly 50 chars long summary line, followed by two newlines, followed by text wrapped to 70 chars. Can you try to rephrase your commit messages accordingly?

Done

  • Can you rebase your edited commits to master?

Done

  • When you post a patch or branch, be sure to change status to needs_review. To be clear, this is not the main reason for us not reviewing sooner, but it could be a possible reason for future delays.

As I wrote on 21 May 2019 "Re: [metrics-team] Sonarqube on metrics-lib" :

So I created the first issue on trac system [1] (Using
try-with-resources or close resource) but I cannot change status to in
progress or assign to me.

Please let me know your code review,
Best Regards

fava

comment:7 Changed 8 weeks ago by karsten

Status: needs_revisionnew

Looks good to me! Merged all three commits and pushed them to master. Sorry for missing the part about not being able to change ticket status, I'll try to remember that. Setting this ticket to new for the remaining try-with-resources cases, unless you tell me there are none anymore. Thanks!

Changed 7 weeks ago by fava

Fix leaking resource TorpertResultImpl

Changed 7 weeks ago by fava

Attachment: actions-trac-30544.png added

Actions trac

comment:8 Changed 7 weeks ago by fava

Hi karsten,

Replying to karsten:

Looks good to me! Merged all three commits and pushed them to master. Sorry for missing the part about not being able to change ticket status, I'll try to remember that. Setting this ticket to new for the remaining try-with-resources cases, unless you tell me there are none anymore. Thanks!

I found another try-with-resources case please find in attachment a patch
0001-Fix-leaking-resource-in-TorperfResultImpl.java.patch​​ for this last case.

I also create my remote branch https://github.com/f-a-v-a/metrics-lib/tree/30544-try-with-resources-2 .

I cannot be able to change ticket status as I sown in snapshot actions-trac-30544.png​​

Let me know
Best Regards

Last edited 7 weeks ago by fava (previous) (diff)

comment:9 Changed 6 weeks ago by karsten

Looks good! Merged to master. Thanks!

Don't worry about ticket status. I receive an email for every change on this ticket, so I'll notice whenever there's something to review. It would just be more convenient to have the status changed to needs_review, because then the ticket would show up in queries, too. But that's not your fault!

Changed 6 weeks ago by fava

comment:10 Changed 6 weeks ago by fava

Replying to karsten:

Looks good! Merged to master. Thanks!

Last compressed archive with all remaining try-with-resource 30544-resource-leak-v4.tar.gz.

I also execute travis + sonarcloud on a branch master + try-with-resources-patches and there are no more issues for try-with-resources .
Please find at latest analysis

Best Regards

comment:11 in reply to:  10 Changed 4 weeks ago by karsten

Resolution: fixed
Status: newclosed

Replying to fava:

[...] there are no more issues for try-with-resources .

Cool! Closing this ticket then. Want to create a new ticket for the next issue type? Thanks!

Note: See TracTickets for help on using tickets.