samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.

http://samtools.github.io/htsjdk/

283 stars 242 forks source link

Fix issue 1414- add proper behavior of clear() to DiskBackedQueue #1415

Closed biotinker closed 5 years ago

biotinker commented 5 years ago

Description

Fix bug in DiskBackedQueue.

DiskBackedQueue.closeIOResources() was using IOUtil.deletePaths() to clean up its temp file, but IOUtil.deletePaths() splits a given path into an array of filenames, and there is no function in IOUtil that would properly remove this one path (without needlessly wrapping the temp file in an iterator).

I copied the try/catch that does the file deletion in IOUtil.deletePaths() to the correct place within closeIOResources, and it now works as expected.

codecov-io commented 5 years ago

Codecov Report

Merging #1415 into master will decrease coverage by 0.008%. The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #1415       +/-   ##
===============================================
- Coverage     68.124%   68.116%   -0.008%     
+ Complexity      8379      8378        -1     
===============================================
  Files            573       573               
  Lines          33982     33986        +4     
  Branches        5668      5668               
===============================================
  Hits           23150     23150               
- Misses          8643      8646        +3     
- Partials        2189      2190        +1

Impacted Files	Coverage Δ	Complexity Δ
...ain/java/htsjdk/samtools/util/DiskBackedQueue.java	`65.487% <100%> (+1.267%)`	`29 <0> (ø)`	:arrow_down:
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java	`80.952% <0%> (-9.524%)`	`3% <0%> (-1%)`
src/main/java/htsjdk/samtools/util/IOUtil.java	`57.627% <0%> (-0.484%)`	`118% <0%> (ø)`

lbergelson commented 5 years ago

@biotinker Thank you for identifying this problem and producing a patch! This pr looks like it solves the problem, but I think in this case changing the underlying problem in the method is justified.

The behavior you identified in IOUtil.deletePaths(Iterable<Path>) is REALLY bad. It was essentially a copy of the old method IOUtil.deleteFile and no one noticed that Path is itself and Iterator<Path>.

Rather than working around the pathological behavior I'm opening a new PR to fix the underlying problem with the method since it's completely broken.

Would you mind reviewing https://github.com/samtools/htsjdk/pull/1416 and making sure it solves your problem?

lbergelson commented 5 years ago

@biotinker I merged the alternate pr that made fixes to the underlying method. Thank you for identifying this and opening a pr to fix it!