Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't hold the underlying file/channel open for longer than necessary. #7

Merged
merged 1 commit into from
Aug 27, 2014
Merged

Don't hold the underlying file/channel open for longer than necessary. #7

merged 1 commit into from
Aug 27, 2014

Conversation

asnare
Copy link
Contributor

@asnare asnare commented Aug 25, 2014

When loading from a database, once read and/or mapped the file/channel can be safely closed.
(Even when using MEMORY_MAPPED, once we have the mapped buffer the original file can be safely closed without affecting the buffer.)

When loading from a database, once read and/or mapped the file/channel can be safely closed.
(Even when using MEMORY_MAPPED, once we have the mapped buffer the original file can be safely closed without affecting the buffer.)
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.19%) when pulling 111d668 on divolte:close-file-immediately-after-loading into 0b91040 on maxmind:master.

@oschwald
Copy link
Member

Thanks. You are right. We should probably just get rid of the Closeable interface. At one point, there was a plan to create a non-mmap file mode, but there hasn't been much demand.

@asnare
Copy link
Contributor Author

asnare commented Aug 26, 2014

The same occurred to me. However before discarding it entirely I thought there might be a way to mitigate the problem you've identified on Windows with the mapping.

Specifically, right now (before and after this pull request), close() has no externally visible affect. Clients can continue to make queries against the database with impunity. I was pondering replacing the reference to BufferHolder with an AtomicReference that could be cleared on close(). This would at least allow GC to reap the underlying buffer, although the exact moment this happens would still be undefined. Once closed, calls to get(InetAddress) could yield ClosedDatabaseException.

My dev platform isn't Windows, but is this something that would interest you? If not, I can refine this pull request by removing the Closeable interface from Reader.

@oschwald
Copy link
Member

I like that suggestion as it doesn't require an API change, and it does provide some utility to the close() method. I suspect it would help some users of the library who are having issues on Windows with file locking. It is a bit weird that the sole purpose of the close() method would be to release a reference, but it does seem like a pragmatic solution.

@asnare
Copy link
Contributor Author

asnare commented Aug 27, 2014

Right. I'll leave this pull-request as-is then (and ready for merge, I hope) and code the AtomicReference change in a separate pull request.

threw = false;
} finally {
try {
// Also closes the underlying channel.
Copy link
Member

Choose a reason for hiding this comment

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

Is this documented somewhere? My Google searches suggest that this may depend on the implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation for RandomAccessFile.close() has included the following since Java 1.4:

If this file has an associated channel then the channel is closed as well.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks. 👍 I must have totally glossed over that.

@oschwald oschwald merged commit 111d668 into maxmind:master Aug 27, 2014
@oschwald
Copy link
Member

I released 0.3.4 with the changes.

@asnare
Copy link
Contributor Author

asnare commented Aug 27, 2014

Thanks; much appreciated!

@oschwald
Copy link
Member

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants