google / agera

Reactive Programming for Android

Apache License 2.0

7.2k stars 639 forks source link

Resource leak fix in test app #143

Closed kushalsharma closed 7 years ago

kushalsharma commented 7 years ago

Should SqlDatabaseFunctions have a way to close database? Strict mode tells me that there is a leak when test app is closed.

googlebot commented 7 years ago

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

:memo: Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

codecov-io commented 7 years ago

Codecov Report

Merging #143 into master will not change coverage. The diff coverage is n/a.

@@           Coverage Diff            @@
##             master    #143   +/-   ##
========================================
  Coverage      97.3%   97.3%           
  Complexity      610     610           
========================================
  Files            39      39           
  Lines          1632    1632           
  Branches        190     190           
========================================
  Hits           1588    1588           
  Misses           13      13           
  Partials         31      31

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 e68d561...9ea15f3. Read the comment docs.

ghost commented 7 years ago

Well spotted! Just a thought though; wouldn't make more sense to pass the database supplier to the store constructor, and call close() on that instead? (still in the method store.close() added in this PR)

googlebot commented 7 years ago

CLAs look good, thanks!

kushalsharma commented 7 years ago

Yes makes sense to hold database supplier as a field and calling close on it, thanks for pointing out. Updated PR.