[TECHNICAL] Stream handling and double-close in UploadFileFromContentUriWorker

[dataCenter430]

Related Issues

In copyFileToLocalStorage():

1. Double-close: outputStream was closed inside outputStream.use { ... }, then outputStream.close() was called again. Closing an already-closed stream is redundant and can cause issues on some implementations.

2. Leak on error: inputStream was not managed with use. If inputStream?.copyTo(fileOut) threw, the input stream was never closed, leaking the underlying content resolver stream.

App: #4798

Solution

• Both streams are now managed with use: the content resolver input with openInputStream(contentUri)?.use { ... }, and the file output with FileOutputStream(cachePath).use { ... }. Both are closed when the block exits, including on exception.

• The explicit inputStream?.close() and outputStream.close() calls were removed so only use is responsible for closing; no double-close.

• Nested use blocks ensure the output stream is closed first, then the input stream; any exception during copyTo still closes both.

• Add changelog files for the fixed issues in folder changelog/unreleased. More info here

[CLAassistant]

All committers have signed the CLA.

[dataCenter430]

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.

hey, @CLAassistant
I've just signed to CLA, can you check it?

[jesmrec]

Thanks for the contribution @dataCenter430 . I'll give you some tips that you will also find in our CONTRIBUTING file:

• For the branch names, we use underscore to separate _ the chunks, like dataCenter430:fix/stream_handling_and_double_close_for_UploadFileFromContentUriWorker . Also, the prefix to use should match the type of the issue. The issue you fixed starts with [TECHNICAL], so, it should be technical/... (that's one of the valid prefixes as the (https://github.com/owncloud/android/blob/master/CONTRIBUTING.md) file). Don't worry for that in the current PR, but please take in account for future contributions 😉. We'd appreciate that.

• For the PR title, we use to add a prefix like [FIX], [BUG], [FEATURE REQUEST]... at the beginning. Check examples in the PR list

• We use to follow Conventional Commits to name the commits. As you'd have realised, the CI checks below ⬇️ shows a red ❌ in the Conventional Commits check. For a quick summary check this. Basically, the commit name must start with a correct prefix (check here the ones that CI verifies). Please fix, you can amend the commit or fix it with an interactive rebase. Any question you could have about this processes just ping us and we will be pleasant to help you!! This is required to merge the PR.

• In terms of CHANGELOG, you added the log file, very appreciated!! just a detail about it: as our guide, we use the present perfect passive tense to name the action. You'll find examples in the changelog/ folder (check unreleased folder for the newest ones). As an extra tip: we'd rather to have only one changelog commit for every PR. So, better to amend or rebase interactively than adding more commits!!

• About the CLA, if you already signed it, i guess in the next pushes it will mark the check as green 🟢

Any question about the process, @joragua or myself will be happy to help you!!

[dataCenter430]

hi, @jesmrec
thanks for your tips. I've just rebased the commits.
could you pls review the PR again?

[jesmrec]

that's good @dataCenter430 . Just an extra small change regarding:

In terms of CHANGELOG, you added the log file, very appreciated!! just a detail about it: as our guide, we use the present perfect passive tense to name the action.

i suggest you the following text:

Unsafe stream handling in UploadFileFromContentUriWorker.copyFileToLocalStorage() has been fixed to prevent resource leaks and redundant close calls.

As soon as it is done, @joragua will review your contribution as soon as he is available. A code review first, and a QA stage later would suggest also changes or improvements in the contribution. Keep tuned!

[dataCenter430]

Hi, @jesmrec
thanks for your review again.
@joragua could you pls review the PR when you have a chance?
Best regards,

[joragua]

I will take care of this as soon as possible 🙌🏻 I will ping you when it's ready

[joragua]

Good job @dataCenter430! 🙌🏻 Some comments here.

NOTE: Regarding the first commit, I'd use refactor: prefix since the code you added is a change in the code but not a fix.

[dataCenter430]

Hi, @joragua thanks for your review.
Very good catch. 👍
I've fixed them you wanted.
Please review the PR when you have a chance.
Best regards.

[joragua]

Thanks for the changes @dataCenter430! Just two comments before doing the second CR round:

1. Please check NOTE: section in the first CR

2. We use to have only one commit for the changelog in a PR, so you can remove the latest one with an interactive rebase and push the commit (chore: add changelog file) with the final version (applying all comments of the first CR)

Let us know if you have any question and we will help you! 😄

[dataCenter430]

Thanks for the changes @dataCenter430! Just two comments before doing the second CR round:

1. Please check NOTE: section in the first CR
2. We use to have only one commit for the changelog in a PR, so you can remove the latest one with an interactive rebase and push the commit (chore: add changelog file) with the final version (applying all comments of the first CR)

Let us know if you have any question and we will help you! 😄

Hi, @joragua
I've changed them as you wanted
Does it work for us?
thanks again. 🙏

[joragua]

One more comment and it's ready @dataCenter430

[joragua]

LGTM now! ✅ Let's move it to QA

[jesmrec]

Approved on my side 🚀

@dataCenter430 please rebase the branch and we will merge your contribution. Thanks a lot for your patience!!

[dataCenter430]

Hi, @jesmrec
Please review the PR when you have a chance.
Thank you so much.

[jesmrec]

@dataCenter430 we use rebase to update the branches instead of merging other branches into, in order to keep the history tidy and sorted.

Could you remove the merge commit and rebase? Here:

https://github.com/owncloud/android/blob/master/CONTRIBUTING.md#3-update-your-contribution-branch-with-master-changes

you'll find how to do it, anyway, if you have any question let us know. Thanks!!

[jesmrec]

Thanks again @dataCenter430 . Merging, the contribution will be available in 4.8.0 🚀

[dataCenter430]

I'm so happy to contribute to Android!
Thank you for your time. 🙏