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

PR 910 introduces regression whereby sorting collection throws exceptions at JVM exit #1029

Closed
tfenne opened this issue Nov 16, 2017 · 5 comments · Fixed by #1032
Closed
Labels

Comments

@tfenne
Copy link
Member

tfenne commented Nov 16, 2017

@magicDGS It looks to me like your PR #910 changes the behavior of temporary file creation and cleanup in a way that breaks lots of downstream usage.

The problem appears to be in DeletePathThread where it attempts to delete a Path and rethrows any exceptions that are encountered. The problem is that one of those exceptions that can be thrown is a NoSuchFileException because the file has already been cleaned up.

Running SortSam using the latest version of Picard I get:

$  java -Xmx4g -jar picard.jar SortSam I=in.bam O=out.bam SO=queryname
...
INFO	2017-11-16 07:50:23	SortSam	Finished reading inputs, merging and writing to output now.
[Thu Nov 16 07:50:27 EST 2017] picard.sam.SortSam done. Elapsed time: 0.18 minutes.
Runtime.totalMemory()=1586495488
Exception in thread "Thread-2" Exception in thread "Thread-1" htsjdk.samtools.util.RuntimeIOException: java.nio.file.NoSuchFileException: /var/folders/mz/34h8j89n1jj2mg6frd0lmqxh0000gn/T/tfenne/sortingcollection.3170510762910173658.tmp
	at htsjdk.samtools.util.IOUtil$DeletePathThread.run(IOUtil.java:374)
Caused by: java.nio.file.NoSuchFileException: /var/folders/mz/34h8j89n1jj2mg6frd0lmqxh0000gn/T/tfenne/sortingcollection.3170510762910173658.tmp
	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
	at sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:244)
	at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103)
	at java.nio.file.Files.delete(Files.java:1126)
	at htsjdk.samtools.util.IOUtil$DeletePathThread.run(IOUtil.java:372)
htsjdk.samtools.util.RuntimeIOException: java.nio.file.NoSuchFileException: /var/folders/mz/34h8j89n1jj2mg6frd0lmqxh0000gn/T/tfenne/sortingcollection.5241745018661624118.tmp
	at htsjdk.samtools.util.IOUtil$DeletePathThread.run(IOUtil.java:374)
Caused by: java.nio.file.NoSuchFileException: /var/folders/mz/34h8j89n1jj2mg6frd0lmqxh0000gn/T/tfenne/sortingcollection.5241745018661624118.tmp
	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
	at sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:244)
	at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103)
	at java.nio.file.Files.delete(Files.java:1126)
	at htsjdk.samtools.util.IOUtil$DeletePathThread.run(IOUtil.java:372)

This is happening, I believe, because SortingCollection has already cleaned up it's temporary files on close, and the belt-and-suspenders of deleting on exit fails because the files are no longer there.

@tfenne tfenne added the bug label Nov 16, 2017
@magicDGS
Copy link
Member

I guess that this can be solved using Files.deleteIfExists in DeletePathThread instead of the current implementation, to do not throw in such a case.

I can't do a PR until next week, so if it's urgent I'll recommend to submit it by yourself. Sorry for the inconvenience...

@yfarjoun
Copy link
Contributor

This also implies that the SortSam isn't tested in a way that forces files to be written to disk...hmmm.

@tfenne
Copy link
Member Author

tfenne commented Nov 16, 2017

@yfarjoun I don't think that's right. I think it's that SortSam isn't tested in a way that the JVM it's running in exits such that it triggers the JVM shutdown hooks that run this code.

@yfarjoun
Copy link
Contributor

ah... good point.

@tbl3rd
Copy link
Contributor

tbl3rd commented Nov 16, 2017

@tfenne @yfarjoun What Tim said. The SortingCollectionTest reliably succeeds, but then the test runner throws like crazy when shutting down.

htsjdk.samtools.util.RuntimeIOException:
  java.nio.file.NoSuchFileException:
    /var/folders/yk/9xqbctq514x_46wjqyh6294jt97pxd/T/tbl/SortingCollectionTest/sortingcollection.8451775603646045897.tmp
	at htsjdk.samtools.util.IOUtil$DeletePathThread.run(IOUtil.java:374)
Caused by: java.nio.file.NoSuchFileException:
  /var/folders/yk/9xqbctq514x_46wjqyh6294jt97pxd/T/tbl/SortingCollectionTest/sortingcollection.8451775603646045897.tmp
	at sun.nio.fs.UnixException.translateToIOException(UnixException.java:86)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:102)
	at sun.nio.fs.UnixException.rethrowAsIOException(UnixException.java:107)
	at sun.nio.fs.UnixFileSystemProvider.implDelete(UnixFileSystemProvider.java:244)
	at sun.nio.fs.AbstractFileSystemProvider.delete(AbstractFileSystemProvider.java:103)
	at java.nio.file.Files.delete(Files.java:1126)
	at htsjdk.samtools.util.IOUtil$DeletePathThread.run(IOUtil.java:372)

lbergelson added a commit that referenced this issue Nov 17, 2017
fixing the crash on shutdown introduced in #910 by changin the command
that gets run on deleteOnExit
Files.delete() -> Files.deleteIfExists()
this will prevent the jvm from crashing on shutdown if the temporary
files get cleaned up before it closes

This still leaves open the possibilty of crash on shutdown if the
temporary file is unable to be removed for some other reason.  That's
subtly different than the File version of deleteOnExit, but it may be
useful to get these error messages in the unusual case of failing to
delete a file that exists.
lbergelson added a commit that referenced this issue Nov 17, 2017
* fixing the crash on shutdown introduced in #910 by changing the command that gets run on on deleteOnExit 
Files.delete() -> Files.deleteIfExists() 
this will prevent the jvm from crashing on shutdown if the temporary files get cleaned up before it closes

* This still leaves open the possibilty of crash on shutdown if the temporary file is unable to be removed for some other reason.  That's subtly different than the File version of deleteOnExit, but it may be useful to get these error messages in the unusual case of failing to delete a file that exists.

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

Successfully merging a pull request may close this issue.

4 participants