proper closing of BufferedWriter in LockFile.java

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.

Trac:
Username: firebrand

added component::metrics/onionoo priority::very low reporter::firebrand resolution::fixed status::closed type::enhancement labels

Trac:
Username: firebrand

onionoo-041015-lockfile.patch

Trac:
Username: firebrand

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.

Trac:
Cc: N/A to iwakeh

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?

Trac:
Username: firebrand

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!

Trac:
Username: firebrand

onionoo-041015-lockfile-2.patch

Trac:
Username: 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.

Trac:
Username: firebrand

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?

@iwakeh good point, I will rework the patch.

Trac:
Username: firebrand

Trac:
Username: firebrand

onionoo-041015-lockfile-3.patch

Trac:
Username: firebrand

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

Trac:
Username: firebrand

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!

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

Trac:
Username: firebrand

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.)

Trac:
Status: new to closed
Resolution: N/A to fixed

closed