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.

Sign up for GitHub

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

Jump to bottom

DiskBackedQueue closeIOResources is broken #1414

Open
biotinker opened this issue Aug 28, 2019 · 4 comments
Open

DiskBackedQueue closeIOResources is broken #1414

biotinker opened this issue Aug 28, 2019 · 4 comments
Labels
bug

Comments

@biotinker
Copy link

Description of the issue:

When calling clear() on a DiskBackedQueue, rather than deleting the file in question, the path to the temp file is split into its constituent pieces and each treated as file names to be deleted- thus failing to delete the intended file, and potentially deleting random other files.

This is due to the use of IOUtil.deletePaths, which calls Arrays.asList() on whatever you pass it, converting our correct Path into an array of the folder names making it up.

Steps to reproduce

Create a DiskBackedQueue with some directory for TMP_DIR, fill it enough that it spills to disk, and then call clear() on it

Expected behaviour

The temp file should be deleted

Actual behaviour

The folder names making up the path to the temp file are treated as file names, and each of these file names attempt deletion from the current working directory.
@lbergelson
Copy link
Member

That sounds nasty... Will fix. Thank you for reporting that.

@lbergelson lbergelson added the bug label Aug 28, 2019
@biotinker
Copy link
Author

Just added a PR with a fix

@biotinker
Copy link
Author

https://github.com/samtools/htsjdk/pull/1415/commits

@lbergelson
Copy link
Member

Oh, that's even better :) . Thank you.

lbergelson added a commit that referenced this issue Aug 29, 2019
@lbergelson
Fixing bad behavior in IOUtil.deletePaths
cf63312 
* The method IOUtil.deletePaths(Iterable<Path>) had pathological.
  * A Path is an Iterable<Path> which iterates over the subpaths in the Path.
     Therefore, this version was binding instead of the varargs version that takes Path...
     Then the method would try to delete all of the subpaths in the path.
  * This is obviously bad behavior.  Fixed by adding an explicit check to see if it's called with a single Path.
  * Extracted a new method deletePath which is used for deleting a single path.

* Fixes ##1414
@lbergelson lbergelson mentioned this issue Aug 29, 2019
Merged
lbergelson added a commit that referenced this issue Aug 30, 2019
@lbergelson
Fixing bad behavior in IOUtil.deletePaths (#1416)
e1bbd34 
* The method IOUtil.deletePaths(Iterable<Path>) had pathological.
  * A Path is an Iterable<Path> which iterates over the subpaths in the Path.
     Therefore, this version was binding instead of the varargs version that takes Path...
     Then the method would try to delete all of the subpaths in the path.
  * This is obviously bad behavior.  Fixed by adding an explicit check to see if it's called with a single Path.
  * Extracted a new method deletePath which is used for deleting a single path.

* Fixes ##1414
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@lbergelson @biotinker