Opened 5 years ago

Closed 4 years ago

#15654 closed enhancement (fixed)

proper closing of BufferedWriter in LockFile.java

Reported by: firebrand Owned by:
Priority: Very Low Milestone:
Component: Metrics/Onionoo Version:
Severity: Keywords:
Cc: iwakeh Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

The BufferedWriter in utils/LockFile.java should be closed inside a finally block. This ensures that the file handle is released even in presence of exceptions.

Child Tickets

Attachments (3)

onionoo-041015-lockfile.patch (1.1 KB) - added by firebrand 5 years ago.
onionoo-041015-lockfile-2.patch (1.1 KB) - added by firebrand 4 years ago.
onionoo-041015-lockfile-3.patch (1.6 KB) - added by firebrand 4 years ago.

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by firebrand

comment:1 Changed 4 years ago by karsten

Cc: iwakeh added

The nested try blocks are not exactly pretty. I wonder, should we switch to try-with-resources? There are probably quite a few similar places in the codebase that we should fix.

comment:2 Changed 4 years ago by firebrand

I think "try-with-resources" is the most elegant way to handle these issues. Should I have a look at the codebase and fix all places that I find?

comment:3 Changed 4 years ago by karsten

Maybe let's start with this one and see if there are any unforeseen issues that require discussion. We can still do the other ones after that. Thanks!

Changed 4 years ago by firebrand

comment:4 Changed 4 years ago by firebrand

Updated the patch to not contain nested try blocks anymore. I took the first two statements out of the existing try block as they cannot throw an IOException anyways. I put them into their own block as they might throw a SecurityException if the location for the lock file is not readable or writable.

comment:5 Changed 4 years ago by iwakeh

Hi firebrand,

thanks for the second patch!

I think it's a good idea to really start using more java 7 features
(like try-with-resources) since onionoo was upgraded a while ago.

The SecurityException should not be discarded without any action.
We don't want to continue in case of a problem here.

The missing comment to the LockFile acquireLock method should
state something like the following:

/**
 * In case of any problem acquiring the lock file  
 * <code>false</code> will be returned.
 * Only successful creation of a previously non-existant
 * lock file will lead to the return value <code>true</code>.
 *
 */

When you look at the usage of acquireLock you'll find
that the program terminates if it receives 'false'.
It is therefore important to return false after catching
any kind of Throwable and to log the error appropriately.

Maybe you could redesign the solution a little?

@karsten: what do you think?

comment:6 Changed 4 years ago by firebrand

@iwakeh good point, I will rework the patch.

Changed 4 years ago by firebrand

comment:7 Changed 4 years ago by firebrand

Reworked the patch to also return false in case of a SecurityException. Also added the JavaDoc comment.

comment:8 Changed 4 years ago by karsten

Thanks for the patch! The code looks good to me and passes local tests, the docs need tweaking. Please find branch task-15654 in my public repository for suggested fixes. If you like them, I'll squash and merge. Thanks!

comment:9 Changed 4 years ago by firebrand

your changes look good, feel free to merge! happy to see my first patch come in.

comment:10 Changed 4 years ago by karsten

Resolution: fixed
Status: newclosed

Great, squashed and merged to master. Thanks again! Resolving.

(If you'd like to improve similar places in the code, please open a new ticket for that.)

Note: See TracTickets for help on using tickets.