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

Modify NitfReading classes to support closing / try with resources #191

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

svargasConnexta
Copy link

I ran into a locked file bug that stemmed from this module while working on integration tests for another project. The problem starts here, a NitfInputStreamReader is created with a new FileInputStream object inside of it. There is no way to close that FileInputStream object other than relying on garbage collection which leads to bugs. This PR is to make the miscellaneous nitf parsing objects closeable to prevent these bugs.

@CLAassistant
Copy link

CLAassistant commented Jan 4, 2019

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Contributor

@bradh bradh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a unit test case, and one minor format thing, otherwise looks good.

@dcruver Any thoughts or concerns?

@@ -125,4 +125,10 @@ public final void skip(final long count) throws NitfFormatException {
throw new NitfFormatException(GENERIC_READ_ERROR_MESSAGE + ex.getMessage(), numBytesRead);
}
}

@Override public void close() throws Exception {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Override would be better on a separate line.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I vaguely remember us running into a problem with closing this stream because it was being passed around within Alliance. I think that since we've added .end() to the fluent classes, we might be ok to use this, now. We should verify that it doesn't cause a problem outside of imaging-nitf, however.

@codecov-io
Copy link

Codecov Report

Merging #191 into master will increase coverage by <.01%.
The diff coverage is 57.14%.

@@             Coverage Diff              @@
##             master     #191      +/-   ##
============================================
+ Coverage     88.14%   88.15%   +<.01%     
- Complexity     2068     2069       +1     
============================================
  Files           199      199              
  Lines          6309     6314       +5     
  Branches        494      494              
============================================
+ Hits           5561     5566       +5     
+ Misses          567      566       -1     
- Partials        181      182       +1

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

Successfully merging this pull request may close these issues.

5 participants