Uncatchable throws in httpSession

[siakc]

We were getting uncaught exceptions from this package (node version v14.21.3). After investigating the matter noticed we have

  public createSession(url: string): http2.ClientHttp2Session {
    if (!this.http2Session || this.isClosed ) {
      const opts: http2.SecureClientSessionOptions = {
        // Set local max concurrent stream limit to respect backend limit
        peerMaxConcurrentStreams: 100,
        ALPNProtocols: ['h2']
      }
      const http2Session = http2.connect(url, opts)

      http2Session.on('goaway', (errorCode, _, opaqueData) => {
        throw new FirebaseAppError(
          AppErrorCodes.NETWORK_ERROR,
          `Error while making requests: GOAWAY - ${opaqueData.toString()}, Error code: ${errorCode}`
        );
      })

      http2Session.on('error', (error) => {
        throw new FirebaseAppError(
          AppErrorCodes.NETWORK_ERROR,
          `Error while making requests: ${error}`
        );
      })
      return http2Session
    }
    return this.http2Session
  }

added in src/utils/api-request.ts (commit: e5001c1). Here when we throw errors in the callbacks there is no way we could catch it with a try...catch or .catch().

[lahirumaramba]

Hi @siakc thank you for filing this issue. Let me look into it. In the meantime, can you share any error logs you might have?

[siakc]

Sure

{"message":"Error while making requests: Error: Client network socket disconnected before secure TLS connection was established","name":"Error","stack":"Error: Error while making requests: Error: Client network socket disconnected before secure TLS connection was established\n    at ClientHttp2Session.<anonymous> (/opt/myapp/node_modules/firebase-admin/lib/utils/api-request.js:997:23)\n    at ClientHttp2Session.emit (events.js:400:28)\n    at emitClose (internal/http2/core.js:1051:10)\n    at processTicksAndRejections (internal/process/task_queues.js:82:21)","code":"app/network-error"},"msg":"Unhandled Exception"}

It maybe helpful to note that the cause of the problem seemed to be a DNS misconfiguration on our side.

[lahirumaramba]

It maybe helpful to note that the cause of the problem seemed to be a DNS misconfiguration on our side.

Thanks for the additional context!
To understand this better, (when you had the DNS misconfiguration) you were unable to catch this exception from your code because the way it was thrown in the callback in the SDK, is that correct?

[siakc]

It maybe helpful to note that the cause of the problem seemed to be a DNS misconfiguration on our side.

Thanks for the additional context! To understand this better, (when you had the DNS misconfiguration) you were unable to catch this exception from your code because the way it was thrown in the callback in the SDK, is that correct?

This is correct.
[EDIT]
My suggestive fix is "to throw in a promise" or "create a documentation to explain a global event catcher is needed to catch those types of exceptions".

[mtrezza]

Maybe related, see logs in parse-community/parse-server-push-adapter#340 (comment). These errors started to appear about 4 weeks ago without code change.

[mtrezza]

My suggestive fix is "to throw in a promise" or "create a documentation to explain a global event catcher is needed to catch those types of exceptions".

These type of network error are often not associated with any specific request. An error like ECONNRESET can occur at any time for a multitude of reasons. Global exception catching would not be convenient for developers and is not good practice for an SDK to require. A better remediation could be:

• Check connection state and try to reestablish if necessary. If reestablishing takes longer than a configurable timeout, invoke a callback (or via an event queue) to notify the developer.
• Identify active requests in process while the connection failed. Queue and retry once the connection is reestablished. The SDK already has a retry and backoff mechanism built-in that maybe could be used.
• Maintain a http/2 connection pool. If one connection has an issue, route requests via another one.

I think the http/2 implementation was an important step, but it now needs to be refined for robustness in order to be a viable alternative to the legacy http/1.1 connection. Until then, the issue can be avoided by not using http/2 with enableLegacyHttpTransport, which comes with the note:

This will be removed when the HTTP/2 transport implementation reaches the same stability as the legacy HTTP/1.1 implementation.

I'd suggest that the SDK hasn't reached said stability yet without http/2 connection error remediation.

[siakc]

If the error is not catchable by developer code because module code is doing stuff independenty (like by handling events or using timers), emitting events or accepting callbacks seem to be the appropriate mechanisms for giving a chance to developer to handle the errors in an asynchronous manner.

[lahirumaramba]

Thanks folks, I think we can reject in a promise... something similar to:

firebase-admin-node/src/utils/api-request.ts

Line 643 in cb374ca

this.enhanceAndReject(err, null, req);

Let me also look into using EventEmitter to emit these events or callbacks (as an alternative) so the developer code can decide how to handle them. This will most likely have to wait till after the holidays.

[mtrezza]

I think we reject in a promise... something similar to:

@lahirumaramba if it helps, see parse-community/parse-server-push-adapter#341 for how we wrapped sendEachForMulticast into a try/catch block in order to have a rejected promise instead of a thrown error. That seems to catch the GOAWAY errors. For ECONNRESET we'd need the emitter / callback approach. To be clear, events or callbacks are not "an alternative" to rejecting a promise, because these errors won't necessarily occur during a promise call, they occur independent of it, see my previous comment.

[ChanHHOO]

I got a same issue.. :(

[whes1015]

I got a same issue,too.

[ksmohamed]

FirebaseAppError: Error while making requests: AggregateError
at ClientHttp2Session. (/app/node_modules/firebase-admin/lib/utils/api-request.js:1012:23)
at ClientHttp2Session.emit (node:events:524:28)
at emitClose (node:internal/http2/core:1115:10)
at process.processTicksAndRejections (node:internal/process/task_queues:90:21)

{
errorInfo: {
code: 'app/network-error',
message: 'Error while making requests: AggregateError'
},
codePrefix: 'app'
}

how to fix this error?

I am trying to use the Firebase Admin SDK in my Node.js app to send a push notification to a specific device. I have the device token and the message I want to send.
I have the following code:

 var message = {
        notification: {
            title: titless,
            body: bodyss
        },
        data: {
            screen: path
        },
        apns: {
            payload: {
                aps: {
                    sound: 'default'
                }
            }
        },
        tokens: fcmTokens,
    };

        const response = await messaging.sendEachForMulticast(message);

[jonathanedey]

Hey Folks, after some investigating we decided to move forward with a promise based solution. We can reject session errors in a promise and let you catch and handle those errors on your side.

Using sendEach().catch(), you would be able to catch session errors as a FirebaseMessagingError with error code HTTP2_SESSION_ERROR.

Since these errors usually occur when some messages may have already been processed or are in flight, we'll also provide a Promise to BatchResponse (if one exists) to access failed and successful requests as part of the error.

[mtrezza]

after some investigating we decided to...

@jonathanedey What was the decision in regards to ECONNRESET errors? These are the real issue, because they are unrelated to developer invoked promises, so not even a try/catch block works. See #2789 (comment) and #2789 (comment).

[lahirumaramba]

@jonathanedey What was the decision in regards to ECONNRESET errors? These are the real issue, because they are unrelated to developer invoked promises, so not even a try/catch block works. See #2789 (comment) and #2789 (comment).

I think there are two layers to this issue. The promised based solution will address the UncaughtException events (first layer) and I think that is a reasonable immediate fix.

As the second layer, we will look into addressing and properly handling ECONNRESET errors on the session (reconnect the session where appropriate etc.) but that will be an improvement unrelated to this fix. The SDK should already retry ECONNRESET errors at the request level (we will also run tests to confirm this that it WAI for HTTP/2 connections)

firebase-admin-node/src/utils/api-request.ts

Line 248 in 63d615f

ioErrorCodes: ['ECONNRESET', 'ETIMEDOUT'],

[mtrezza]

Ok, maybe let's call them request vs. session errors. I'm unsure which of the two this issue originally focused on. Request errors surely should be handled via a rejected promise instead of throwing. For session errors I would like to stress that handling them is not a mere "improvement" but a fundamental requirement for a robust HTTP/2 lib. They are currently not handled, and they cannot be handled as part of a promise, because they may occur at any time. There are several error frames (not just ECONNRESET) that can be sent at any time by the server, due to the nature of HTTP/2 maintaining a persistent connection between the client and the server. Even though a HTTP/2 GOAWAY is more likely to be sent upon request, it can be sent by the server also suddenly without any request being made, for example if the server rotates.

[lahirumaramba]

@mtrezza I see your point! The SDK connects an HTTP/2 session only during a call to a sendEach* function and closes the session once the function is done. Any session errors can be handled as part of the promise during the call to sendEach*.

From what I understand, session errors technically should not occur when the session is not connected, but I might be wrong here. We can look into adding callback support to cover those potential cases.

[mtrezza]

I believe if the HTTP/2 session is in fact completely closed after the sendEach* call, then it may be possible to actually handle any error in the promise. Good point.

On a separate note, however, that brings up the question, whether the session should be closed after each call. It's one of the advantages of HTTP/2 that for frequent calls, maintaining a persistent pool of connections would be beneficial over opening and closing connections each time. Just want to mention it, but surely out of scope for this issue here.

[joachimbulow]

This is a major issue - what can we do to work around?
Our pods are constantly crashing atm - surely a library this common should not blindly emit errors to crash the event loop.

We are reverting to using enableLegacyHttpTransport to hopefully resolve the issue for now

[mtrezza]

I believe flagging the HTTP/2 implementation as "experimental" would have been a less confusing choice than flagging the working HTTP/1 implementation as "legacy" at this stage. For many, like @joachimbulow, the assumption is that they should migrate to HTTP/2 because HTTP/1 is legacy and will be removed soon. Unaware of the risks because the HTTP/2 implementation is really in an "alpha" state right now. In other words, it's confusing to declare an API as "legacy" when there is no other stable API available yet. I think the best guidance for everyone coming across this thread would be (invented quote):

No worries, it's totally fine at this stage to keep using HTTP/1 with enableLegacyHttpTransport: true, but feel free to experiment with HTTP/2 and please provide feedback for improvements and any bugs you may encounter.

[jonathanedey]

Hey folks, with v13.3.0 this issue should now be resolved.

Additionally, we've added a wiki page to help document these changes as well as to show how session errors can be handled.

Currently, there's no intention to fully drop the HTTP/1.1 fallback and we encourage you continue to use this option if you are experiencing issues with HTTP/2. Marking enableLegacyHttpTransport as deprecated, intends to show that it is a temporary method as we continue to improve our HTTP/2 implementation.

We understand your frustrations and your feedback here is valuable as we work towards these improvements.