Firebase Storage `ref.delete()` is emitting exception

russellwheatley commented 2 months ago

[READ] Step 1: Are you in the right place?

Issues filed here should be about bugs in the code in this repository. If you have a general question, need help debugging, or fall into some other category use one of these other channels:

For general technical questions, post a question on StackOverflow with the firebase tag.
For general Firebase discussion, use the firebase-talk google group.
For help troubleshooting your application that does not fall under one of the above categories, reach out to the personalized Firebase support channel.

[REQUIRED] Step 2: Describe your environment

Android Studio version: 2022.3.1
Firebase Component: Storage (Database, Firestore, Storage, Functions, etc)
Component version: 20.3.0

[REQUIRED] Step 3: Describe the problem

If you try to delete a reference that does not exist, an exception is still emitted even when using addOnCompleteListener or addOnFailureListener listener .

The logs might be intended but it's not something I've seen often in other API failures on firebase android.

Logcat logs

``` 2024-04-05 08:15:35.822 14354-17160 StorageException io....lugins.firebasestorageexample E StorageException has occurred. Object does not exist at location. Code: -13010 HttpResult: 404 2024-04-05 08:15:35.826 14354-17160 StorageException io....lugins.firebasestorageexample E { "error": { "code": 404, "message": "Not Found." }} java.io.IOException: { "error": { "code": 404, "message": "Not Found." }} at com.google.firebase.storage.network.NetworkRequest.parseResponse(NetworkRequest.java:415) at com.google.firebase.storage.network.NetworkRequest.parseErrorResponse(NetworkRequest.java:432) at com.google.firebase.storage.network.NetworkRequest.processResponseStream(NetworkRequest.java:423) at com.google.firebase.storage.network.NetworkRequest.performRequest(NetworkRequest.java:265) at com.google.firebase.storage.network.NetworkRequest.performRequest(NetworkRequest.java:282) at com.google.firebase.storage.internal.ExponentialBackoffSender.sendWithExponentialBackoff(ExponentialBackoffSender.java:76) at com.google.firebase.storage.internal.ExponentialBackoffSender.sendWithExponentialBackoff(ExponentialBackoffSender.java:68) at com.google.firebase.storage.DeleteStorageTask.run(DeleteStorageTask.java:59) at com.google.firebase.concurrent.LimitedConcurrencyExecutor.lambda$decorate$0$com-google-firebase-concurrent-LimitedConcurrencyExecutor(LimitedConcurrencyExecutor.java:65) at com.google.firebase.concurrent.LimitedConcurrencyExecutor$$ExternalSyntheticLambda0.run(Unknown Source:4) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) at com.google.firebase.concurrent.CustomThreadFactory.lambda$newThread$0$com-google-firebase-concurrent-CustomThreadFactory(CustomThreadFactory.java:47) at com.google.firebase.concurrent.CustomThreadFactory$$ExternalSyntheticLambda0.run(Unknown Source:4) at java.lang.Thread.run(Thread.java:919) ```

Steps to reproduce:

Run the code below
See above noted logs in logcat.

Relevant Code:

Java snippet:

FirebaseStorage.getInstance().getReference().child("non-existent-ref.png")
        .delete()
        .addOnCompleteListener(
          task -> {
            if (task.isSuccessful()) {
              Log.d("tag", "referenceDelete: success");
            } else {
              Log.d("tag", "referenceDelete: failed");
            }
          }).addOnFailureListener(e -> {
          Log.d("tag", "referenceDelete: addOnFailureListener" + e);
        });

argzdev commented 2 months ago

Hey @russellwheatley, thanks for the well detailed report. I was able to exhibit the same behavior. I'll reach out to our engineers about this.

puf commented 2 months ago

Hey Russel (@russellwheatley),

While it's an interesting idea to have the SDKs suppress logging of these errors once the developer attached a failure or completion listener, I don't think we do that anywhere else. I'm also not sure if this is a pattern we should introduce.

We have no reasonable way to verify that the developer actually handles the error (although they indicate they do in your original FlutterFire issue #12590). Given the number of completion handlers that I see where devs simply ignore the failure case, we might end up in a worse situation when trying to help the troubleshoot problems in their app.

Do we have a method that checks whether a file exists at a reference? That would be a much better way of handling use-case, as such a method of course would not raise/log an error when the file doesn't exist. If such a method doesn't exist, should it?

Levi-Lesches commented 2 months ago

I checked for such a method and I don't think it exists based on the Firebase docs. The Flutter docs indicate that metadata.size may be null, which leads me to believe it would be null on a non-existent reference, but the [Android SDK equivalent](https://firebase.google.com/docs/reference/kotlin/com/google/firebase/storage/StorageMetadata#getSizeBytes()) is non-nullable so that could be wrong.

In any case an exists method would be helpful, and would be a way to avoid this issue.

Re: logging, I don't think it's a good idea to emit logs on all error, especially full tracebacks. Aside from the fact that the tracebacks are not useful for Flutter users, throwing errors will print them to the console anyway. If a dev explicitly handles it, then it's a non-issue for their app because they supposedly have logic to ignore or work around the error. My use-case is deleting an object, and ignoring "not found" errors. This makes sense because the goal is to ensure the object doesn't exist, so if it was never there then there's no problem. Having such thorough tracebacks each time fills up my console with irrelevant information I specifically chose to ignore in the code. Basically,

We have no reasonable way to verify that the developer actually handles the error

I would argue that letting the error go unhandled is just as noticeable as logging it directly, unless I'm missing something about Android dev in Java/Kotlin workflows where errors can go unhandled silently.

argzdev commented 2 months ago

Hi @Levi-Lesches, thank you for the feedback. This is very insightful. We've given this some thought, and we could consider having an exists method as a feature request. It would be most beneficial for our fellow developers since it can be used for other use cases while also addressing the root cause of the problem. I'll go ahead and mark this as a feature request. While we are unable to promise any timeline for this, we'll definitely keep this under our radar.

P.S. For folks who find this useful, adding an emoji thumbs up on the original post can help us prioritize adding this to the roadmap.

firebase / firebase-android-sdk