Opened 3 months ago

Last modified 6 weeks ago

#30544 needs_revision defect

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 (2)

try-with-resources.csv (3.7 KB) - added by fava 3 months ago.
Sonarqube try-with-resources
patch-fix-resource-leak.zip (3.3 KB) - added by fava 3 months ago.

Download all attachments as: .zip

Change History (7)

Changed 3 months ago by fava

Attachment: try-with-resources.csv added

Sonarqube try-with-resources

comment:1 Changed 3 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 3 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 3 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 3 months ago by fava

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

comment:4 in reply to:  1 Changed 3 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 3 months ago by fava (previous) (diff)

comment:5 Changed 6 weeks 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!

Note: See TracTickets for help on using tickets.