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.