firebase / firebase-admin-dotnet

Firebase Admin .NET SDK
https://firebase.google.com/docs/admin/setup
Apache License 2.0
366 stars 131 forks source link

Reduce ambiguity surrounding exception handling. #272

Closed n0n11n closed 3 years ago

n0n11n commented 3 years ago

When trying to implement exception handling for Firebase Cloud Messaging I encountered some ambiguity that could be addressed with better documentation or comments and accessibility changes.

Specifically this code example: else if (ex.ErrorCode == ErrorCode.Unavailable) { // Platform-level error code Console.WriteLine("FCM service is temporarily unavailable"); ScheduleForRetry(notification, TimeSpan.FromHours(1)); }

I think this example should use ex.MessagingErrorCode == MessagingErrorCode.Unavailable instead of ex.ErrorCode == ErrorCode.Unavailable.

Also ScheduleForRetry example or the documentation should mention that the SDK already implements some retry functionality of its own and you have to wait for the SDK's retries to finish before you can handle the error yourself.

I think the RetryOptions should be made accessible so you wouldn't have to wait 2 min (in the worst case) till the SDK returns the 503 Unavailable error.

google-oss-bot commented 3 years ago

I found a few problems with this issue:

hiranya911 commented 3 years ago

I think this example should use ex.MessagingErrorCode == MessagingErrorCode.Unavailable instead of ex.ErrorCode == ErrorCode.Unavailable.

I assume you're referring to the examples in https://firebase.google.com/docs/reference/admin/error-handling. This particular example is intended to demonstrate checking for both platform-level and service-level error codes in the same try-catch block. Therefore the way it is written currently is correct. Although I agree that from a practical implementation point of view checking for MessagingErrorCode.Unavailable is more appropriate for that use case.

Also ScheduleForRetry example or the documentation should mention that the SDK already implements some retry functionality of its own and you have to wait for the SDK's retries to finish before you can handle the error yourself.

SDK's built-in retry happens before any exceptions are thrown. So by the time the exception is caught and handled in user code, the SDK has already finished its retries.

I think the RetryOptions should be made accessible so you wouldn't have to wait 2 min (in the worst case) till the SDK returns the 503 Unavailable error.

That sounds like a good feature request. We can look into making RetryOptions configurable in the future.

n0n11n commented 3 years ago

I assume you're referring to the examples in https://firebase.google.com/docs/reference/admin/error-handling. This particular >example is intended to demonstrate checking for both platform-level and service-level error codes in the same try-catch block. >Therefore the way it is written currently is correct. Although I agree that from a practical implementation point of view checking >for MessagingErrorCode.Unavailable is more appropriate for that use case.

Yes I meant that example and I should have added a link to the file. I get now why it is written the way it is. I was only looking at it from my use case point of view at the time.

SDK's built-in retry happens before any exceptions are thrown. So by the time the exception is caught and handled in user >code, the SDK has already finished its retries.

I meant that this information "SDK retries then it throws" could be made more prominent in either the code example or the documentation of the SDK or both. If you want I can make a PR to show what I mean if this explanation doesn't make sense. 🙂

We can look into making RetryOptions configurable in the future.

I would appreciate it! 🥳

hiranya911 commented 3 years ago

I meant that this information "SDK retries then it throws" could be made more prominent in either the code example or the documentation of the SDK or both.

We do have a line about that on the same page. The "Automatic Retries" section starts with the sentence: "The Admin SDK automatically retries certain errors before exposing those errors to the users."

I'm open to mentioning this in the code samples too. @egilmorez wdyt?

n0n11n commented 3 years ago

We do have a line about that on the same page. The "Automatic Retries" section starts with the sentence: "The Admin SDK >automatically retries certain errors before exposing those errors to the users."

I must have missed that. That is actually very clear. If you still add that info as a comment to the code, then that is great but as far as I am concerned this issue can then be closed. Thanks for considering my suggestions.