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

Don't close archive when FileObject streams close #6

Merged
merged 1 commit into from
Dec 31, 2020

Conversation

pclewis
Copy link
Contributor

@pclewis pclewis commented Dec 15, 2020

Currently, if you open an input stream for a file and then close it,
the entire archive is closed, but VFS will keep trying to use it, and
any future input streams you open will just close immediately (because
junrar silently swallows an NPE in the pipe writer thread).

The built-in providers have this same notifyAllStreamsClosed() method
commented out. It does not seem like correct behavior.

@gotson
Copy link
Member

gotson commented Dec 23, 2020

Would you be able to provide a sample code or a failing test of the current behaviour?

Also, your PR doesn't pass CheckStyle.

Currently, if you open an input stream for a file and then close it,
the entire archive is closed, but VFS will keep trying to use it, and
any future input streams you open will just close immediately (because
junrar silently swallows an NPE in the pipe writer thread).

The built-in providers have this same notifyAllStreamsClosed() method
commented out. It does not seem like correct behavior.
@pclewis pclewis force-pushed the dont-close-archive-when-file-closed branch from 4ba6154 to 943c4ca Compare December 28, 2020 17:05
@codecov
Copy link

codecov bot commented Dec 28, 2020

Codecov Report

Merging #6 (e887917) into master (aa7cc25) will increase coverage by 0.94%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master       #6      +/-   ##
============================================
+ Coverage     60.76%   61.71%   +0.94%     
  Complexity       28       28              
============================================
  Files             6        6              
  Lines           130      128       -2     
  Branches          8        8              
============================================
  Hits             79       79              
+ Misses           46       44       -2     
  Partials          5        5              
Impacted Files Coverage Δ Complexity Δ
...github/junrar/vfs2/provider/rar/RARFileSystem.java 64.15% <ø> (+2.33%) 7.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aa7cc25...943c4ca. Read the comment docs.

@pclewis
Copy link
Contributor Author

pclewis commented Dec 28, 2020

The change to the existing test demonstrates the issue. There are no exceptions, but the second input stream is empty:

RarVfsTest > givenRarFile_whenAccessingViaVFS_thenChildrenAreAvailable() FAILED
    org.opentest4j.AssertionFailedError: 
    Expecting:
     <"">
    to be equal to:
     <"file2
    ">
    but was not.

@gotson gotson merged commit 080826c into junrar:master Dec 31, 2020
github-actions bot pushed a commit that referenced this pull request Dec 31, 2020
## [1.1.1](v1.1.0...v1.1.1) (2020-12-31)

### Bug Fixes

* don't close archive when FileObject streams close ([#6](#6)) ([080826c](080826c))
@github-actions
Copy link

🎉 This PR is included in version 1.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@pclewis pclewis deleted the dont-close-archive-when-file-closed branch December 31, 2020 23:18
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 this pull request may close these issues.

2 participants