poketim commented 4 years ago

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: https://github.com/timkoon/briefcase/commit/915e262d78ef2e1a32ceb3e3bfedb9346798a0d9 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 commented 4 years ago

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 commented 4 years ago

@getodk/briefcase

lognaturel commented 4 years ago

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 https://github.com/getodk/briefcase/blob/7bb6185e6c3212e9dbe791df32fde25b21ab4a29/src/org/opendatakit/briefcase/reused/http/RequestBuilder.java#L111 and below to close them by using try with resources. Or is it Request.body that never gets closed?

codecov-commenter commented 4 years ago

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 commented 4 years ago

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

https://github.com/getodk/briefcase/blob/7bb6185e6c3212e9dbe791df32fde25b21ab4a29/src/org/opendatakit/briefcase/reused/http/RequestBuilder.java#L111

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: https://github.com/getodk/briefcase/blob/7bb6185e6c3212e9dbe791df32fde25b21ab4a29/src/org/opendatakit/briefcase/reused/transfer/AggregateServer.java#L160

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.

ggalmazor commented 4 years ago

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

poketim commented 3 years ago

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

lognaturel commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

getodk / briefcase

Close response InputStreams #865

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

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

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?

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.

Codecov Report

Codecov Report

Codecov Report