Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow custom retry config and retry disabling in Firebase App options #1739

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

thiagomorales
Copy link

@thiagomorales thiagomorales commented Jun 3, 2022

Description

This PR will allow customize the retry config, or completely disable it by passing the RetryConfig or disableRetry in Firebase App initializating options as following:

New Firebase App Options:

disableRetry?: boolean;
retryConfig?: RetryConfig;
export interface RetryConfig {
    backOffFactor?: number;
    ioErrorCodes?: string[];
    maxDelayInMillis: number;
    maxRetries: number;
    statusCodes?: number[];
}

These retry options will be injected to HttpClient and will be used in api calls.

In addition, we have allow customize the api requests timeout by an optional environment variable, keeping the same default values:

FIREBASE_AUTH_TIMEOUT=25000
FIREBASE_MESSAGING_BATCH_TIMEOUT=10000
FIREBASE_MESSAGING_TIMEOUT=10000
FIREBASE_PROJECT_MANAGEMENT_TIMEOUT=10000

Context

Today, the retry config is an internal piece of HttpClient and the users can't change or disable this behaviour.

I've experienced some weird scenarios with push messages duplications even receiving 'ETIMEDOUT' and HTTP 503 errors from FCM backend, like seen here: #1248

@google-cla
Copy link

google-cla bot commented Jun 3, 2022

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).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@thiagomorales
Copy link
Author

Hi @lahirumaramba! Could you take a look here please?
Thanks in advance.

@lahirumaramba lahirumaramba self-requested a review July 11, 2022 20:43
@lahirumaramba lahirumaramba self-assigned this Jul 11, 2022
@RichieViscardi
Copy link

RichieViscardi commented Aug 2, 2022

@lahirumaramba Any update on this? The 10 second timeout is causing my app to get duplicate notifications - if this can be customized that would be a lifesaver!!

@thopedam
Copy link

thopedam commented Aug 5, 2022

This is seriously such a nice fix for a super annoying problem @RichieViscardi mentioned, which began appearing recently. I tried pushing this to the firebase-support, but they don't really understand this issue.

@lahirumaramba
Copy link
Member

Hi @thiagomorales thank you for the contribution!
Thank you everyone, this PR includes changes to the public API surface and has to go through an internal review process before we can merge it. I can't promise a timeline, but let me talk with the team and initiate the review process.

@RichieViscardi
Copy link

That would be great thank you! This is a temporary work around but as @thopedam said, this timeout issue has just started appearing recently. Topic messages are being sent, but it is acting as if they are timing out so a retry is still happening. I have contacted Firebase Support and they have put in a case for me on this issue

@frytg
Copy link

frytg commented Aug 25, 2022

How's the status on the Timeout/Retry issue? We've observed this exact some problem with one of our projects as well. Some requests are taking up to 30-60sec, which considering the internal 10sec timeout produces duplicate messages for end users.
@RichieViscardi did you hear back from Firebase Support or should we also open a ticket to get some more attention to this?

@chong-shao's old comment in #1248 suggests that Firebase backend errors can occasionally spike, causing this problem?

@RichieViscardi
Copy link

@frytg I really haven't heard anything from Support yet. They reached out to me again like 10 days ago asking for more information - but they have been silent since then. This is definitely a major issue and it is frustrating that it seems nobody cares too much about it. I was able to come up with a workaround by just setting our firebase function to a 10 second timeout. So the function kills which prevents any duplicates from being sent - this has been working but of course is not the ideal solution

@thopedam
Copy link

@RichieViscardi How have you been doing this? Unfortunately this does not always prevent from multiple deliveries on my side.. reduces this issue drastically though.

@@ -83,6 +84,16 @@ export interface AppOptions {
* specifying an HTTP Agent in the corresponding factory methods.
*/
httpAgent?: Agent;

/**

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably add more details on the meaning of the fields in RetryConfig? E.g. how to interpret backoffFactor

@lahirumaramba
Copy link
Member

Hey everyone, thank you for your patience on this! We have recently increased the timeout for FCM to 15 seconds as a way to address the duplicate notifications issue. However, we think having a custom retry config is even better. We have initiated the internal API review process for the public API surface changes. Stay tuned!

@rjam
Copy link

rjam commented Mar 31, 2023

Hey everyone, thank you for your patience on this! We have recently increased the timeout for FCM to 15 seconds as a way to address the duplicate notifications issue. However, we think having a custom retry config is even better. We have initiated the internal API review process for the public API surface changes. Stay tuned!

@lahirumaramba Has any sort of custom retry config made it into the sdk? This would still be very useful but I can't find it still.

@ThingUroboros
Copy link

ThingUroboros commented Oct 31, 2023

@lahirumaramba Hi, when are you planning to merge the changes from this MR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants