Correctly close resources in BitmapUtils. #440

vanniktech · 2022-09-26T09:35:51Z

             StrictMode  D  StrictMode policy violation: android.os.strictmode.LeakedClosableViolation: A resource was acquired at attached stack trace but never released. See java.io.Closeable for information on avoiding resourc
                            e leaks.
                         D      at android.os.StrictMode$AndroidCloseGuardReporter.report(StrictMode.java:1986)
                         D      at dalvik.system.CloseGuard.warnIfOpen(CloseGuard.java:336)
                         D      at android.os.ParcelFileDescriptor.finalize(ParcelFileDescriptor.java:1069)
                         D      at java.lang.Daemons$FinalizerDaemon.doFinalize(Daemons.java:319)
                         D      at java.lang.Daemons$FinalizerDaemon.runInternal(Daemons.java:306)
                         D      at java.lang.Daemons$Daemon.run(Daemons.java:140)
                         D      at java.lang.Thread.run(Thread.java:1012)
                         D  Caused by: java.lang.Throwable: Explicit termination method 'close' not called
                         D      at dalvik.system.CloseGuard.openWithCallSite(CloseGuard.java:288)
                         D      at dalvik.system.CloseGuard.open(CloseGuard.java:257)
                         D      at android.os.ParcelFileDescriptor.<init>(ParcelFileDescriptor.java:206)
                         D      at android.os.ParcelFileDescriptor.<init>(ParcelFileDescriptor.java:189)
                         D      at android.os.ParcelFileDescriptor.open(ParcelFileDescriptor.java:232)
                         D      at androidx.core.content.FileProvider.openFile(FileProvider.java:632)
                         D      at android.content.ContentProvider.openAssetFile(ContentProvider.java:2073)
                         D      at android.content.ContentProvider.openTypedAssetFile(ContentProvider.java:2249)
                         D      at android.content.ContentProvider.openTypedAssetFile(ContentProvider.java:2316)
                         D      at android.content.ContentProvider$Transport.openTypedAssetFile(ContentProvider.java:561)
                         D      at android.content.ContentResolver.openTypedAssetFileDescriptor(ContentResolver.java:2027)
                         D      at android.content.ContentResolver.openAssetFileDescriptor(ContentResolver.java:1842)
                         D      at android.content.ContentResolver.openInputStream(ContentResolver.java:1518)
                         D      at com.canhub.cropper.BitmapUtils.decodeSampledBitmapRegion(BitmapUtils.kt:730)
                         D      at com.canhub.cropper.BitmapUtils.cropBitmap(BitmapUtils.kt:550)
                         D      at com.canhub.cropper.BitmapUtils.cropBitmap(BitmapUtils.kt:268)
                         D      at com.canhub.cropper.BitmapCroppingWorkerJob$start$1.invokeSuspend(BitmapCroppingWorkerJob.kt:48)
                         D      at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
                         D      at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
                         D      at kotlinx.coroutines.scheduling.CoroutineScheduler.runSafely(CoroutineScheduler.kt:570)
                         D      at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.executeTask(CoroutineScheduler.kt:749)
                         D      at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.runWorker(CoroutineScheduler.kt:677)
                         D      at kotlinx.coroutines.scheduling.CoroutineScheduler$Worker.run(CoroutineScheduler.kt:664)

vanniktech · 2022-09-26T09:36:12Z

cropper/src/main/java/com/canhub/cropper/BitmapUtils.kt

                }
-            } while (options.inSampleSize <= 512)
-
-            closeSafe(stream)


There was no try / catch / finally here

cropper/src/main/java/com/canhub/cropper/BitmapUtils.kt

+                        } catch (e: OutOfMemoryError) {
+                            options.inSampleSize *= 2
+                        }
+                    } while (options.inSampleSize <= 512)


Canato

Love the simplification and changes, not sure how to test it for the edge cases or catch that was removed and how you checked if the streams didn't keep live.

Besides this, happy with the changes,

CHANGELOG.md

Canato · 2022-09-26T10:00:50Z

cropper/src/main/java/com/canhub/cropper/BitmapUtils.kt

+        return context.contentResolver.openOutputStream(newUri!!, WRITE_AND_TRUNCATE).use {
+            bitmap.compress(compressFormat, compressQuality, it)
+            newUri


What happens here if newUri is null or the compress method fails?

#440 (comment)

Canato · 2022-09-26T10:04:30Z

cropper/src/main/java/com/canhub/cropper/BitmapUtils.kt

-            val decoder = BitmapRegionDecoder.newInstance(stream!!, false)
-            do {
+
+            context.contentResolver.openInputStream(uri).use {


Is this stream closed automatically? I could not find something that guarantee it.

Have you looked at the implementation of use?

I didn't know the use of the keyword use either. I just looked it up. This is the given description of use in kotlin.

Executes the given block function on this resource and then closes it down correctly whether an exception is thrown or not.

Links for more info

Kotlin use documentation

A stackoverflow explanation of use

vanniktech · 2022-09-26T10:17:22Z

@Canato first of all this PR does not fix #439 - I don't know why you're making this assumption.

Addressed the changelog.

Canato · 2022-09-26T10:20:57Z

this PR doe s not fix #439 - I don't know why you're making this assumption.

@vanniktech
Because you have issues following templates, like the one we have for PRs. I thought this would be connected since you opened this PR after raising the issue, but was planning to check

I decided to put it here as a reminder for myself instead of closing your PR.

I will let it open, and when I understand how to check this stuff and what was wrong first, I review it.

vanniktech · 2022-09-26T10:30:00Z

Why are you so fame addicted? It seriously baffles me.
With you it's always about stars, number of monthly downloads, following some templates which don't always necessary apply or some other bullshit.

Why isn't the focus of this library to provide good APIs? It's such a hassle contributing to this library in any way.

Please correct me if I'm wrong but the description of this pull request clearly shows a stacktrace of StrictMode. You know what that is right?

I've even commented the exact line which was the problem - #440 (comment)

What else do you need?

Canato · 2022-09-26T11:50:28Z

I firmly believe you need to chill a little bit and be more flexible, maybe some time outside can help. Not being ironic or anything to attack you is just a suggestion from someone you don't know, so, of course, feel free to ignore it.
Just not sure why you decide to personal attack me since you don't even know me.

Now trying to answer you.

Why are you, so fame addicted?

Not sure how to ask you to use a template inside a PR inside a repo no one else will check make me fame addicted.

it's always about stars, the number of monthly downloads

Not sure how many successful open-source projects you've been able to be part of since day one. For me, this is the first, and I'm learning on the way without much help, but it is working so far
This "starts, numbers, download" has helped me personally to know my effort here is helping people, and I'm not just spending time without valuable results and have helped people to know this is a library that is growing, stable and being maintained.

following some templates which don't always necessary apply

I'm happy to improve the templates and would love some suggestions. What I put in place is what I know can help me yo optimize my time while working on the project. What doesn't apply doesn't need to be filled, of course.

some other bullshit

Happy to remove bullshit if you can point me to where it is something that does not need to be done.

Why isn't the focus of this library to provide good APIs?

Yes, there is something in the library that is not achieving that we could improve?

It's such a hassle contributing to this library in any way.

The nice stuff about OSS is that you can fork and make a better version of it where people will find it better to use it. This would make me happy and remove all my concerns. I can add on the README a link to your fork if you want.

I am upset that you feel this way. That's not my goal.
I spent a lot of personal time putting this project where it is today from a project that was abandoned.
I expect to create a nice community and workforce to help to maintain this. I probably I'm failing at this.
But I would love to have more people answering other persons issues, reviewing other peoples PRs, helping to improve the release process etc.

But most people get here and just care about what they are passing without thinking about the next and the whole project as OSS community.

Please correct me if I'm wrong but the description of this pull request clearly shows a stacktrace of StrictMode. You know what that is right?

No, I don't know what a StrictMode, in this case, is.
I don't know why you opened this or what approach you took to fix it.
As I don't know how to test, how you tested and what you trying to achieve

You can see that I don't know much about stuff, and I'm happy to learn from someone better on the topic than me (you in this case), but I cannot learn assuming a lot of stuff that I don't know.

So I need to ask for some more context, or I will spend time studying and investigating. This is not a problem but will make it take longer. Where, since you know, you can share.
But templates feel unnecessary

The template for PRs has 3 points

Which issue close (if any)
Description of what you are trying to achieve/fix/do
A check list for the code reviewer, not you, to help me to review (or any other person who would review it but in the whole history of this repo it never happened, hope this change)

Without the first, I already make a wrong assessment of the focus of the PR, etc

What else do you need?

Believe our issue is that we don't understand each other point of view.
This video is a presentation I did about the process of handing over the library. Sorry for my English, it was a long in my life until I had the opportunity to learn when I left my country,...

That said, I'm in Berlin this week until Thursday, and then I go to Munich for Oktoberfest. It would be nice to catch up if you can
vcanato@gmail.com

Not sure when I will return to this PR.

vanniktech · 2022-09-26T14:26:58Z

My reply was not meant to be an attack in any way. Just straight forward to the point.

Not sure how to ask you to use a template inside a PR inside a repo no one else will check make me fame addicted.

It's always about numbers with you - to show a few examples:

#439 (comment)
#396 (comment)
https://github.com/CanHub/Android-Image-Cropper/releases/tag/4.3.1

Why enforce a template when you get a perfectly valid PR or issue? By enforcing that template and closing issues / PR that don't match your template, you're basically communicating: Thanks but no thanks. (That is the bullshit I'm talking about; always nitpicking)

It's the same thing with that bot. There are issues, that are valid and triaged and then that bot wants to autoclose it? Just seems a way to close things out, which reduces number of open issues, which looks 'better'.

Yes, there is something in the library that is not achieving that we could improve?

A lot. I've tried here #396 and eventually gave up.

Being an open source maintainer sucks most of the time. You're 99.9% of the time alone. When you get a contribution, you should be an enabler. Instead you're nit picking about templates.

It just seems you're motivated by how many users use this library. And the less time you spend on it the better for you. I do appreciate the time and effort you put into it! I know it's not easy, I have a few open source libraries myself.

Regarding strict mode, Google has some documentation: https://developer.android.com/reference/android/os/StrictMode I have a similar snippet in my app and StrictMode crashes since the stream from this library isn't closed properly. How did I fix it? Well, #440 (comment) was the underlying issue and I fixed it with Kotlin's use extension function which closes the stream regardless of whether an exception was thrown or not. You did this in the other places manually by try / catch / finally. Kotlin has better APIs though.

That said, I'm in Berlin this week until Thursday, and then I go to Munich for Oktoberfest. It would be nice to catch up if you can vcanato@gmail.com

That's cool! Unfortunately, I'm in Essen at the moment and it's quite far away.

Canato · 2022-09-27T08:23:20Z

https://github.com/CanHub/Android-Image-Cropper/discussions/441

vanniktech requested a review from a team as a code owner September 26, 2022 09:35

vanniktech commented Sep 26, 2022

View reviewed changes

github-advanced-security bot found potential problems Sep 26, 2022

View reviewed changes

Correctly close resources in BitmapUtils.

f9e4161

Canato requested changes Sep 26, 2022

View reviewed changes

Canato linked an issue Sep 26, 2022 that may be closed by this pull request

[BUG] - CropRect has mixed up width / height ⚠️ ⚠️ #439

Closed

Code Review.

8c5a11a

vanniktech mentioned this pull request Sep 26, 2022

[BUG] - CropRect has mixed up width / height ⚠️ ⚠️ #439

Closed

Canato removed a link to an issue Sep 26, 2022

[BUG] - CropRect has mixed up width / height ⚠️ ⚠️ #439

Closed

vanniktech mentioned this pull request Sep 28, 2022

Correctly rotate and flip image using Exif attributes. #443

Closed

vanniktech merged commit f1da57d into CanHub:main Oct 19, 2022

vanniktech deleted the close branch October 19, 2022 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correctly close resources in BitmapUtils. #440

Correctly close resources in BitmapUtils. #440

vanniktech commented Sep 26, 2022

vanniktech Sep 26, 2022

Canato left a comment

Canato Sep 26, 2022

vanniktech Sep 26, 2022

Canato Sep 26, 2022

vanniktech Sep 26, 2022

Devenom1 Sep 26, 2022

vanniktech commented Sep 26, 2022

Canato commented Sep 26, 2022

vanniktech commented Sep 26, 2022

Canato commented Sep 26, 2022

vanniktech commented Sep 26, 2022

Canato commented Sep 27, 2022

Correctly close resources in BitmapUtils. #440

Correctly close resources in BitmapUtils. #440

Conversation

vanniktech commented Sep 26, 2022

vanniktech Sep 26, 2022

Choose a reason for hiding this comment

Canato left a comment

Choose a reason for hiding this comment

Canato Sep 26, 2022

Choose a reason for hiding this comment

vanniktech Sep 26, 2022

Choose a reason for hiding this comment

Canato Sep 26, 2022

Choose a reason for hiding this comment

vanniktech Sep 26, 2022

Choose a reason for hiding this comment

Devenom1 Sep 26, 2022

Choose a reason for hiding this comment

vanniktech commented Sep 26, 2022

Canato commented Sep 26, 2022

vanniktech commented Sep 26, 2022

Canato commented Sep 26, 2022

vanniktech commented Sep 26, 2022

Canato commented Sep 27, 2022