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

Added Lck-File that is correctly deleted on JVM-Exit. #173

Merged
merged 4 commits into from
Oct 31, 2016

Conversation

DaKaLKa
Copy link

@DaKaLKa DaKaLKa commented Oct 11, 2016

I'm using sqlite-jdbc with windows and I have a big problem with #80.
As this is a bug in JVM (not able to delete On Exit dlls that were loaded) I added another file beside the extracted dll with the same name and the suffix ".lck".
The file is correctly deleted on JVM exit. So now I can very easy create a cleanup method looking for all sqlite dll with missing lck-file and delete them in custom code.

Other ways (like looking if file is locked) was not always solving the problem (with multiple JVM-processes (e.g. 20)).
(Maybe a cleanup method should also be added)

Maybe this would be useful for other people.
What do you think.

@gitblit
Copy link
Collaborator

gitblit commented Oct 11, 2016

I think adding the lck file as a flag is a step forward, but now we need the corresponding search & delete functionality to make the lck file worthwhile.

@DaKaLKa
Copy link
Author

DaKaLKa commented Oct 11, 2016

Ok, I've added some logic for automatic cleanup. The cleanup is made on exit of the Java-VM, so it has no impact of the startup-time.

@Override
public void run()
{
System.gc();
Copy link
Owner

Choose a reason for hiding this comment

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

Why do you need to call System.gc here?

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, this was from an earlier test.

@xerial
Copy link
Owner

xerial commented Oct 11, 2016

Btw does it actually solve your problem? JVM has no guarantee to call the cleanup shutdown hook.

@gitblit
Copy link
Collaborator

gitblit commented Oct 12, 2016

My initial thought had been to automatically cleanup on startup. Not sure what is best but there have been several reports of massive disk usage because the libs are not properly deleted.

@xerial
Copy link
Owner

xerial commented Oct 12, 2016

@gitblit Agreed. The cleanup phase should be on startup, rather than relying on shutdown hook.

…alization before extracting the nativ library
@DaKaLKa
Copy link
Author

DaKaLKa commented Oct 12, 2016

Now, it's called on startup, before extracting the native library.
For me, it's working lime a charm

@xerial
Copy link
Owner

xerial commented Oct 12, 2016

@DaKaLKa ok. Thanks. First, could you fix the indentation or remove unnecessary blank lines?

loadSQLiteNativeLibrary();
return extracted;
// only cleanup before first extract
if(!extracted) {
Copy link
Owner

Choose a reason for hiding this comment

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

weird indentation



/**
* Deleted old nativ libraries e.g. on Windows this are not removed on VM-Exit (bug #80)
Copy link
Owner

Choose a reason for hiding this comment

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

typo: native

* Deleted old nativ libraries e.g. on Windows this are not removed on VM-Exit (bug #80)
*/
static void cleanup(){

Copy link
Owner

Choose a reason for hiding this comment

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

remove this line

*/
static void cleanup(){

final String ver = org.sqlite.SQLiteJDBCLoader.getVersion();
Copy link
Owner

Choose a reason for hiding this comment

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

unnecessary final and remove org.sqlite.SQLiteJDBCLoder

static void cleanup(){

final String ver = org.sqlite.SQLiteJDBCLoader.getVersion();
String tempFolder = new java.io.File(System.getProperty("java.io.tmpdir")).getAbsolutePath();
Copy link
Owner

Choose a reason for hiding this comment

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

Just import java.io.File

java.io.File [] files = dir.listFiles(new java.io.FilenameFilter() {
@Override
public boolean accept(java.io.File dir, String name) {
return name.startsWith("sqlite-" + ver);
Copy link
Owner

Choose a reason for hiding this comment

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

Use private variable e.g., private final String prefix ="sqlite-" + ver to minimize String generation cost.

}
});

for (java.io.File tempFile : files) {
Copy link
Owner

Choose a reason for hiding this comment

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

listFiles can return null so this might throw NullPointerException

tempFile.delete();
}
catch(SecurityException ex){

Copy link
Owner

Choose a reason for hiding this comment

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

Swallowing exception is a bad habit. At least we should show the error message here.

@DaKaLKa
Copy link
Author

DaKaLKa commented Oct 13, 2016

Hi xerial, I modified the code with your review wishes.
Are there any code formatting specification that you are using. I tried the code formatting from eclipse (eclipde neon) but in doesn't seems to fit 100% to the code formatting you are using. Any settings I should be aware of in further code commits?

@xerial
Copy link
Owner

xerial commented Oct 13, 2016

Thanks. Currently we are not enforcing any specific format. But it will worth a try so that you can find the expected coding style.

I'll check the code later.

@xerial xerial merged commit 176449c into xerial:master Oct 31, 2016
@xerial
Copy link
Owner

xerial commented Oct 31, 2016

Merged to master. Thanks

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.

3 participants