firebase-admin-node icon indicating copy to clipboard operation
firebase-admin-node copied to clipboard

Allow custom retry config and retry disabling in Firebase App options

Open thiagomorales opened this issue 2 years ago • 14 comments

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: https://github.com/firebase/firebase-admin-node/issues/1248

thiagomorales avatar Jun 03 '22 14:06 thiagomorales

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.

google-cla[bot] avatar Jun 03 '22 14:06 google-cla[bot]

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

thiagomorales avatar Jul 10 '22 22:07 thiagomorales

@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!!

RichieViscardi avatar Aug 02 '22 00:08 RichieViscardi

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.

thopedam avatar Aug 05 '22 17:08 thopedam

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.

lahirumaramba avatar Aug 05 '22 18:08 lahirumaramba

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

RichieViscardi avatar Aug 05 '22 19:08 RichieViscardi

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?

frytg avatar Aug 25 '22 10:08 frytg

@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

RichieViscardi avatar Aug 26 '22 14:08 RichieViscardi

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

thopedam avatar Aug 26 '22 14:08 thopedam

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 avatar Oct 27 '22 19:10 lahirumaramba

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.

rjam avatar Mar 31 '23 14:03 rjam

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

ThingUroboros avatar Oct 31 '23 23:10 ThingUroboros