Close response InputStreams

[poketim]

Closes #848

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

Code complies, all tests pass, and application runs.

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

As discussed in the issue, closing the InputStreams after we received the response will help to reduce the number of files opened during a large number of submissions or attachments.

Other approaches were thought of, but would require more re-work/refactoring of existing functionality, where this current solution fits in line with how the code is organized and utilizes InputStreams from the class UncheckedFiles.

I attempted to update and refactor CentralServer using the similar closeInputStream method, but ran into 3 failing unit tests. Code can be found here: poketim@915e262
That change may be verified in a separate pull request as issue 848 just references AggregateServer.

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?

The changes should reduce the errors regarding too many files opened. There shouldn't be other impacts to a user's experience. As a note, after the PushToAggregate operation receives a response, it now does some cleaning up (closing input streams and clearing the tracked streams opened).

Does this change require updates to documentation? If so, please file an issue at https://github.com/opendatakit/docs/issues/new and include the link below.

I don't believe so.

[codecov-io]

Codecov Report

Merging #865 into master will increase coverage by 0.02%.
The diff coverage is 91.66%.

@@             Coverage Diff              @@
##             master     #865      +/-   ##
============================================
+ Coverage     48.28%   48.31%   +0.02%     
- Complexity     1641     1644       +3     
============================================
  Files           195      195              
  Lines         10394    10413      +19     
  Branches        751      753       +2     
============================================
+ Hits           5019     5031      +12     
- Misses         5018     5024       +6     
- Partials        357      358       +1     

Impacted Files	Coverage Δ	Complexity Δ
...g/opendatakit/briefcase/reused/UncheckedFiles.java	45.16% <60.00%> (+0.62%)	32.00 <1.00> (+1.00)
...akit/briefcase/push/aggregate/PushToAggregate.java	75.43% <100.00%> (+1.85%)	30.00 <0.00> (+2.00)
...kit/briefcase/reused/transfer/AggregateServer.java	86.43% <100.00%> (+0.42%)	68.00 <5.00> (+1.00)
.../opendatakit/briefcase/util/BadFormDefinition.java	0.00% <0.00%> (-33.34%)	0.00% <0.00%> (-1.00%)
src/org/opendatakit/briefcase/util/FormCache.java	82.27% <0.00%> (-2.54%)	19.00% <0.00%> (ø%)
...takit/briefcase/model/BriefcaseFormDefinition.java	33.14% <0.00%> (-1.69%)	16.00% <0.00%> (-1.00%)
...g/opendatakit/briefcase/ui/export/ExportPanel.java	48.07% <0.00%> (+1.92%)	14.00% <0.00%> (+1.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 4b7130a...544430b. Read the comment docs.

[poketim]

@getodk/briefcase

[lognaturel]

Thank you so much for taking this on, @timkoon, and sorry about the radio silence!

I am unfortunately not very familiar with this code. One thing that I've been wanting to verify is that it indeed is necessary to keep track of all streams and close them all at once at the end. Did you verify that the streams are indeed still open at that point? I would have expected the methods in RequestBuilder at

briefcase/src/org/opendatakit/briefcase/reused/http/RequestBuilder.java

Line 111 in 7bb6185

private static XmlElement readXmlElement(InputStream in) {

and below to close them by using try with resources. Or is it Request.body that never gets closed?

[ggalmazor]

This is looking good to me, although I have some reservations about making the server class aware of the file streams that some of its requests open. While I believe this alleviates the original issue, I think this should be something that the Request should provide instead of the server, along the lines of "file resources opened by this request" so that the HTTP client can close them automatically once the request is sent.

Having said that, I think this doesn't block the release of this PR, although I would consider opening a new PR to address the design improvements I've mentioned above.

I'm also missing tests for this change, which would be nice to have.

[codecov-commenter]

Codecov Report

Merging #865 into master will increase coverage by 0.03%.
The diff coverage is 82.35%.

@@             Coverage Diff              @@
##             master     #865      +/-   ##
============================================
+ Coverage     48.33%   48.36%   +0.03%     
- Complexity     1647     1648       +1     
============================================
  Files           195      195              
  Lines         10422    10438      +16     
  Branches        753      754       +1     
============================================
+ Hits           5037     5048      +11     
- Misses         5026     5030       +4     
- Partials        359      360       +1     

Impacted Files	Coverage Δ	Complexity Δ
...opendatakit/briefcase/reused/http/CommonsHttp.java	54.32% <66.66%> (+1.54%)	12.00 <1.00> (+1.00)
...akit/briefcase/push/aggregate/PushToAggregate.java	73.58% <100.00%> (ø)	28.00 <0.00> (ø)
...g/opendatakit/briefcase/reused/UncheckedFiles.java	49.20% <100.00%> (+2.98%)	34.00 <2.00> (+2.00)
...g/opendatakit/briefcase/ui/export/ExportPanel.java	48.07% <0.00%> (-1.93%)	14.00% <0.00%> (-1.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 b55a171...6c965b9. Read the comment docs.

[poketim]

Thank you so much for taking this on, @timkoon, and sorry about the radio silence!

I am unfortunately not very familiar with this code. One thing that I've been wanting to verify is that it indeed is necessary to keep track of all streams and close them all at once at the end. Did you verify that the streams are indeed still open at that point? I would have expected the methods in RequestBuilder at

briefcase/src/org/opendatakit/briefcase/reused/http/RequestBuilder.java

Line 111 in 7bb6185

private static XmlElement readXmlElement(InputStream in) {

and below to close them by using try with resources. Or is it Request.body that never gets closed?

Hi @lognaturel and no problem! In regards to pointing out the line in the RequestBuilder, I would think that approach is optimal when it comes to attachments in the request, but attachments in the request object are actually using UncheckedFiles.newInputStream. That method isn't using try-with-resources, so the InputStreams created will need to be closed and are causing the exception in #848 .

Here is where I found Aggregate server is calling that static method:

briefcase/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java

Line 160 in 7bb6185

newInputStream(formFile)

I haven't verified that the InputStreams are still open while the application is running, but that could possibly be done in an integration test. I did include unit tests to verify Java InputStreams are working as expected, and that inputStream.close() closes and deallocates files/resources back to the system.

@ggalmazor Thanks for your review and suggestion. I do like your suggestion of making the Client handle closing InputStreams rather than the Server. After reviewing the flow again, I found the Request is already tracking the attachments and InputStreams. I reverted the previous fix and implemented changes in the new commits.

[lognaturel]

This looks like a great improvement to me, @timkoon! It's now very clear which streams could be left open. The change feels nicely targeted and tested.

I'll give @ggalmazor a little bit in case he has comments too.

[ggalmazor]

(Apologies! will have some cycles for this during the weekend)

[ggalmazor]

Thanks for considering my suggestions. I think the implementation is ready, but I've seen some things in the new test class that should be addressed, so I'm requesting some changes there. Feel free to ping me anytime if you think I could help you with that ;)

[poketim]

@ggalmazor Do you have a chance to review the updates?

[lognaturel]

Thanks so much for thoughtfully addressing the review feedback, @timkoon, and for the ping on this. I'm very happy with this as-is and would like to merge. @ggalmazor I can't seem to do so without your approval, though!

[codecov-io]

Codecov Report

Merging #865 into master will increase coverage by 0.02%.
The diff coverage is 92.30%.

@@             Coverage Diff              @@
##             master     #865      +/-   ##
============================================
+ Coverage     48.33%   48.35%   +0.02%     
- Complexity     1647     1649       +2     
============================================
  Files           195      195              
  Lines         10422    10435      +13     
  Branches        753      754       +1     
============================================
+ Hits           5037     5046       +9     
- Misses         5026     5029       +3     
- Partials        359      360       +1     

Impacted Files	Coverage Δ	Complexity Δ
...opendatakit/briefcase/reused/http/CommonsHttp.java	55.12% <83.33%> (+2.35%)	12.00 <1.00> (+1.00)
...g/opendatakit/briefcase/reused/UncheckedFiles.java	47.58% <100.00%> (+1.36%)	33.00 <1.00> (+1.00)
...org/opendatakit/briefcase/reused/http/Request.java	72.22% <100.00%> (+1.63%)	16.00 <1.00> (+1.00)
...g/opendatakit/briefcase/ui/export/ExportPanel.java	48.07% <0.00%> (-1.93%)	14.00% <0.00%> (-1.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 b55a171...62f552b. Read the comment docs.

[ggalmazor]

Hi, @timkoon, @lognaturel!

MEGA Apologies for the delay in this review. This is not how I like to do things, and I hope you can forgive me.

This PR solves the issue at hand but I think there were a couple of small things we needed to change to align the changes with previous code and make it a smidge simpler if possible. I've gone ahead and applied the changes myself to avoid delaying the PR anymore. I hope you understand.

Other than that: LGTM!

[lognaturel]

Thanks both so much! And again, really appreciate your patience, @timkoon. Briefcase v1.18.0 is now out with this fix. 🎉 https://forum.getodk.org/t/odk-briefcase-v1-18/31039