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

Close streams to avoid resource leaks #579

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

lognaturel
Copy link
Member

Now that Collect checks for unclosed streams, I saw that form deserialization always left an open InputStream. I searched for Stream across the project and used try with resources.

What has been done to verify that this works as intended?

Ran all tests and verify that they pass. Visually inspected the code.

Why is this the best possible solution? Were any other approaches considered?

I considered only making the FormDefCache change that I saw logged but I figure that it's a good time to change all of the cases I can find.

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

There should be no user-facing changes other than a slightly reduced risk of resource leaks.

I think this is quite low risk and easy to visually verify but if there is a problem with the code, loading a form def from xml or cache, localization and selects with choice filters are at risk.

Do we need any specific form for testing your changes? If so, please attach one.

No.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

No.

@codecov-commenter
Copy link

Codecov Report

Merging #579 into master will decrease coverage by 0.00%.
The diff coverage is 20.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #579      +/-   ##
============================================
- Coverage     54.53%   54.53%   -0.01%     
  Complexity     3219     3219              
============================================
  Files           244      244              
  Lines         13341    13342       +1     
  Branches       2563     2563              
============================================
  Hits           7276     7276              
- Misses         5256     5257       +1     
  Partials        809      809              
Impacted Files Coverage Δ Complexity Δ
...g/javarosa/core/reference/ReferenceDataSource.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...vices/storage/util/DummyIndexedStorageUtility.java 6.79% <0.00%> (-0.07%) 2.00 <0.00> (ø)
...a/org/javarosa/xml/InternalDataInstanceParser.java 46.66% <100.00%> (ø) 1.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 36f9331...7628ab0. Read the comment docs.

@seadowg seadowg merged commit f146630 into getodk:master Jun 2, 2020
@lognaturel lognaturel deleted the close-streams branch June 5, 2020 04:42
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.

3 participants