Skip to content

FileHandle: don't unnecessarily init a RandomAccessFile on close#307

Merged
ctrueden merged 3 commits intomasterfrom
fix-filehandle-close
Nov 21, 2017
Merged

FileHandle: don't unnecessarily init a RandomAccessFile on close#307
ctrueden merged 3 commits intomasterfrom
fix-filehandle-close

Conversation

@gab1one
Copy link
Contributor

@gab1one gab1one commented Nov 17, 2017

If we close a handle on a file that does not exist, we do not need to
initialize the RandomAccessFile, just to close it again. This fixes the problem of
empty files appearing when closing a FileHandle to a file that does not exist.

If we close a handle on a file that does not exist, we do not need to
initialize the RandomAccessFile, just to close it again. This fixes the problem of
empty files appearing when closing a FileHandle to a file that does not exist.
@gab1one gab1one requested a review from ctrueden November 17, 2017 13:03
Copy link
Member

@ctrueden ctrueden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this simple fix can result in a race condition if close() is called near-simultaneously with another thread that causes the RandomAccessFile to be initialized.

Suppose Thread1 calls fileHandle.writeShort(555); and Thread2 then calls fileHandle.close() while Thread1's operation is in the middle of the new RandomAccessFile call. The raf reference has not yet been assigned, so Thread2's close operation will do nothing.

Maybe this doesn't matter, because RandomAccessFile is not thread-safe anyway. So fileHandle really should not be shared between Thread1 and Thread2.

If we do care for some reason, the only easy fix I can see is to add a new method like getRAF() which is synchronized, and returns raf. This would minimize the chances of (but not fully prevent!) close() doing nothing while another thread is in the midst of the initRAF() operation.

What do you think?

@gab1one
Copy link
Contributor Author

gab1one commented Nov 20, 2017

I don't think a single handle should be shared between threads at all, as this can very easily result in data corruption unless the threads are coordinating their access somehow.

@gab1one gab1one force-pushed the fix-filehandle-close branch from 1f4575a to fdfc4ef Compare November 20, 2017 09:51
@gab1one
Copy link
Contributor Author

gab1one commented Nov 20, 2017

@ctrueden with the added synchronized access do you mean something like I did in commit fdfc4ef ?
That should protect us from the scenario with the closing of a FileHandle that is just initializing.

@ctrueden
Copy link
Member

@gab1one Let's just make the close() method synchronized. Simpler, less buggy, no less efficient. No?

This change does two things:

* It synchronizes close(), so that the RAF cannot be in the process of
  initializing while the close() operation is happening.
* It guards all operations against an already-closed state, such that
  an IOException will be thrown if the handle has already been closed.
@ctrueden ctrueden force-pushed the fix-filehandle-close branch from fdfc4ef to 5f4b56a Compare November 21, 2017 15:09
@ctrueden ctrueden merged commit 7c345eb into master Nov 21, 2017
@ctrueden ctrueden deleted the fix-filehandle-close branch November 21, 2017 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants