temp files existance check fix

[boscogh]

Have a look, this is the solution for issue 59 https://github.com/mikiobraun/jblas/issues/59

The problem was in fact that because OutputStream tries to create same file with same name on each start of jblas, even if file is already there, jblass fails to start in more than 1 instance of the JVM process. It is not necessery, I think.

With this patch I am able to run several instances of jblas at the same time on my Win7 machine. Also, I noticed you used Shutdown hook for deleting files, I think same job could be achieved more easily with java File.deleteOnExit() method. (I didn't touch that code)

cheers

[mikiobraun]

Hi boscogh

Thanks for having a look at this matter.

Reusing existing files was something I considered, but the issue here is that if you start multiple copies at the same time you still get errors because there is no locking between processes.

Then there is the issue that you cannot delete a file on windows when someone else is using it - unlike on UNIX where this is safe and the file actually gets deleted when the last open file handle is closed.

I alap remember considering deleteOnExit but if I recall correctly there were also some issues on windows.

Over all I think the safest bet is to create individual temp directories, or explicitly copy the required files in your path. In that case jblas should load them without unpacking the files from the jar.

I admit it has been some time since I last had a look, so I might be wrong.

What do you think?

-M

----- Ursprüngliche Nachricht ----- Von: "boscogh" notifications@github.com Gesendet: ‎03.‎05.‎2015 12:51 An: "mikiobraun/jblas" jblas@noreply.github.com Betreff: [jblas] temp files existance check fix (#61)

boscogh wants to merge 1 commit into mikiobraun:master from boscogh:master: Have a look, this is the solution for issue 59

59

The problem was in fact that because OutputStream tries to create same file with same name on each start of jblas, even if file is already there. It is not necessery, I think. With this patch I am able to run several instances of jblas at the same time on my Win7 machine. Also, I noticed you used Shutdown hook for deleting files, I think same job could be achieved more easily with java File.deleteOnExit() method. (I didn't touch that code) cheers

You can view, comment on, or merge this pull request online at: https://github.com/mikiobraun/jblas/pull/61 Commit Summary temp files existance check fix File Changes M src/main/java/org/jblas/util/LibraryLoader.java (26) Patch Links: https://github.com/mikiobraun/jblas/pull/61.patch https://github.com/mikiobraun/jblas/pull/61.diff — Reply to this email directly or view it on GitHub.

[boscogh]

Well, so I think I need to correct (widen) this patch to only delete files which were created in LibraryLoader. Then the only first instance of jblas which created them, will delete them. I'll do it in a moment. thank you for caution. Right now (with patch) jblas shows a warning on exit that it couldn't delete files. (this is on Win7), so the correct behavior, I guess will not led to any warnings.

Btw, I found several uses of FileOutputStream, in files like xxxMatrix.java, method save, but only in FloatMatrix.java this stream is closed within the method, all other could create a resource leak (and load method acts the same)

[mikiobraun]

The problem is that the jblas instance which created the files might not be the last one to finish. So it cannot delete the files because instances which came later are still depending on the files.

Thanks for pointing out the other unclosed streams error. I think someone else already reported that one.

Again, I think the cleanest way is to have separate temp dirs.

-M

----- Ursprüngliche Nachricht ----- Von: "boscogh" notifications@github.com Gesendet: ‎03.‎05.‎2015 13:33 An: "mikiobraun/jblas" jblas@noreply.github.com Cc: "Mikio L. Braun" mikiobraun@gmail.com Betreff: Re: [jblas] temp files existance check fix (#61)

Well, so I think I need to correct (widen) this patch to only delete files which were created in LibraryLoader. Then the only first instance of jblas which created them, will delete them. I'll do it in a moment. thank you for caution. Right now (with patch) jblas shows a warning on exit that it couldn't delete files. (this is on Win7), so the correct behavior, I guess will not led to any warnings. Btw, I found several uses of FileOutputStream, in files like xxxMatrix.java, method save, but only in FloatMatrix.java this stream is closed within the method, all other could create a resource leak (and load method acts the same) — Reply to this email directly or view it on GitHub.

[boscogh]

Ok, I understand. I did changed this fix, to delete files created by this concrete instance of JBLAS, not touching temp dir creation logic. can share it, if you think it make sence. 1 thing I noticed during testing - File.delete always return false, (JVM v8), and files are staying alive. but if I use Files.delete(path) (Files, not File) it does the job.

now I'm curious, why if it created all that separate temp dirs before this patch, it was unable to start multiple instances.. I also see a huge amount of jblasXXXXX.dll laying in my temp dir, in total counts for >1GB (during 1 week of different testings..)

was this temp dir creation logic in 1.2.3 release or it is more recent change?

[boscogh]

Sorry for kind of spamming. In my last comment I was wrong in thinking that Files does the job. It also fails with "access denied" on Windows. My guess it because of dll still stay in memory and therefore hold the link to it original file. I tried to mark them as "deleteOnExit" but you were right, it is also fails.

so, to conclude: if you creates a temp dir every time on jblas start, and unload dll's in there, I believe all this dll's would stay on Windows in this temp dir until they get manually deleted. in Linux they are easily deleted, as you pointed out because they are not locked by FS.

ever you can leave it as is, or mb, I think a better idea to upload files every time at the same place, and let only started application to delete them. in this case if there would be any running instances, they will not suffer at all, and any newly started instance of jblas will create a copy of dll/so and use them. seems that I closed the ring..and now I suppose the patch is still worth to make. will wait your comment. thank you for having your time for discussion :)

[mikiobraun]

Actually, I think that the issue on Windows might be that once the dlls are loaded, files are not closed until after the JVM exits, so there is no way to delete these files at all from within the running JVM, even in the shutdown hook.

One idea I had yesterday would be to scan for old jblas files on startup and delete those. The only issue here would be to make sure files don't get deleted in the splitsecond between being copied and being loaded.

Hm. Maybe interleaving the close and the load command will make sure that that doesn't happen on Windows... .

-M