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

Requests not Throwing Errors in NestJS App or AWS Lambda Node 20/18.x #2231

Open
Marlrus opened this issue Nov 18, 2024 · 13 comments
Open

Requests not Throwing Errors in NestJS App or AWS Lambda Node 20/18.x #2231

Marlrus opened this issue Nov 18, 2024 · 13 comments
Assignees
Labels

Comments

@Marlrus
Copy link

Marlrus commented Nov 18, 2024

Describe the bug

Within NestJS, when performing calls such as StripeClient.invoices.create or other operations with bad data, requests hang indefinitely and don't throw errors.

To Reproduce

  1. Instantiate Stripe Client with new Stripe(<API_KEY>)
  2. Wrap API call in try/catch block
  3. Call invoices.create method with an invalid payload (Such as the one in the snippet)
  4. Call will hang indefinitely neither erroring or succeeding

Expected behavior

When an API call fails on any non 2xx status, it should throw an exception that can be caught by a try/catch block or a .catch.

Code snippets

// Inside of Controller/Service/Injectable Service
const StripeClient = new Stripe('<API_KEY>');

try {
  const res = await StripeClient.invoices.create({
    customer: 'non-existent-id',
    collection_method: 'send_invoice',
    days_until_due: 14,
    description: 'desc',
    payment_settings: {
      payment_method_types: ['us_bank_account'],
    },
  });

  console.log(res);
} catch (err) {
  console.error(err);
}

OS

Running on Docker linux/amd64 node:20-alpine

Node version

Node v20.18.0

Library version

stripe node version 17.3.0

API version

Default (Whatever you get when you don't specify any)

Additional context

Noticed running in a NestJS Application running in docker.

I've tried:

  • Running requests through an Injectable Service that instantiates Stripe
  • Running the client directly in the Controller, or Service
  • Attempting to pass httpClient property in new Stripe with Axios and NestJS default HTTP Clients, to no avail.

I also tried writing a script with the exact same code on the exact same version of node on a loose .ts file and running it with ts-node and it worked.

I'll keep plowing through as it is critical for me to know when Stripe operations fail. If I find a solution, I will post it here.

@Marlrus Marlrus added the bug label Nov 18, 2024
@Marlrus
Copy link
Author

Marlrus commented Nov 19, 2024

I've ran some tests and have only managed a middle ground.

I looked at the following comment (#1846 (comment)) which had the following code:

import Stripe from "stripe";

const myHttpClient = Stripe.createNodeHttpClient()
const makeRequest = myHttpClient.makeRequest.bind(myHttpClient)
myHttpClient.makeRequest =(...args) => {
  return makeRequest(...args).then((res) => {
    if (res.getStatusCode() >= 400) {
      callYourCustomErrorHandlerHere(res)
    }
    return res
  })
}}
const stripe = new Stripe("sk_test_xyz", { httpClient: myHttpClient });

I applied something similar:

httpClient.makeRequest = (...args) => {
  return makeRequest(...args).then((res: Stripe.HttpClientResponse) => {
    const stream = res.toStream(() => console.log('STREAM COMPLETED'));

    stream.on('end', () => console.log('END'));
    stream.on('data', () => console.log('DATA'));
    stream.on('response', () => console.log('RESPONSE'));
    stream.on('error', () => console.log('ERROR'));

    res
      .toJSON()
      .then((res) => console.log({ res }))
      .catch((err) => console.log({ err }))
      .finally(() => console.log('FINALLY'));

    const raw = res.getRawResponse();

    raw.on('end', () => console.log('END'));
    raw.on('data', () => console.log('DATA'));
    raw.on('response', () => console.log('RESPONSE'));
    raw.on('error', () => console.log('ERROR'));

    if (res.getStatusCode() > 399) {
      throw res;
    }
    return res;
  });
};
this.client = new Stripe(secretKey, { httpClient });

With the code above, the following is observed:

  • res.toJSON() never resolves on then, catch, or finally.
  • Raw response on event functions never trigger.
  • toStream(), which returns the raw response as well, on event functions never trigger and the streamCompleteCallback is never called.
  • Only getStatusCode() and getHeaders() work.

I dug into the source code and think the following is happening:

  • toJSON never completes because _res no longer has .on events at that point because the NodeHttpClient.makeRequest function is a promise that resolves when the request is completed and the response has already gone through all of the events.
export class NodeHttpClientResponse extends HttpClientResponse {
    constructor(res) {
        // @ts-ignore
        super(res.statusCode, res.headers || {});
        this._res = res;
    }
    getRawResponse() {
        return this._res;
    }
    toStream(streamCompleteCallback) {
        // The raw response is itself the stream, so we just return that. To be
        // backwards compatible, we should invoke the streamCompleteCallback only
        // once the stream has been fully consumed.
        this._res.once('end', () => streamCompleteCallback());
        return this._res;
    }
    toJSON() {
        return new Promise((resolve, reject) => {
            let response = '';
            this._res.setEncoding('utf8');
            this._res.on('data', (chunk) => {
                response += chunk;
            });
            this._res.once('end', () => {
                try {
                    resolve(JSON.parse(response));
                }
                catch (e) {
                    reject(e);
                }
            });
        });
    }
}

On the flip side, my open telemetry instrumentation for the http package is showing the response body... but I cannot access the data in the server which is where I need it to know how to react to an error.

'@opentelemetry/instrumentation-http': {
  responseHook: (span, response: any) => {
    try {
      if (response.statusCode >= 400 || response.status_code >= 400) {
        let body = '';
        response.on('data', (chunk: Buffer) => {
          body += chunk.toString();
        });
        response.on('end', () => {
          span.setAttribute('http.response.body', body);
          response.removeAllListeners();
        });
      }
      if (response.statusCode > 499 || response.status_code > 499) {
        span.setStatus({
          code: Otel.SpanStatusCode.ERROR,
          message: response.message || response.statusMessage || 'No error message',
        });
      }
    } catch (err) {
      console.log(err);
      span.setStatus({
        code: Otel.SpanStatusCode.ERROR,
        message: 'OTEL instrumentation-http responseHook Error',
      });
    }
  },
},

As it stands now, I am in the following situation:

  • Error responses are at least throwing.
  • OTEL instrumentation can catch the error response bodies.
  • I cannot access error details in code.
  • All errors are the same in my catch blocks:
{
  "info": {
    "scope": "POST::/api/v1/subscriptions::SubscriptionService::createSubscription",
    "name": "Error",
    "message": "An error occurred with our connection to Stripe. Request was retried 2 times.",
    "stack": "Error: An error occurred with our connection to Stripe. Request was retried 2 times.\n    at /app/node_modules/stripe/cjs/RequestSender.js:401:41\n    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)"
  },
  "level": "error",
  "message": "An error occurred with our connection to Stripe. Request was retried 2 times.",
  "trace_id": "f6d9cd02242b57463ba2d9d2619978fc",
  "span_id": "898804a2e1e6b5cc",
  "trace_flags": "01"
}

@Marlrus Marlrus changed the title Requests not Throwing Errors in NestJS App Requests not Throwing Errors in NestJS App or AWS Lambda Node 20.x Nov 20, 2024
@Marlrus
Copy link
Author

Marlrus commented Nov 20, 2024

I'm expanding the bug to AWS Lambda as I am experiencing the same behavior.

Describe the bug

When deploying a Lambda to AWS that calls the Stripe package, errors are not throwing.

To Reproduce

  • Instantiate Stripe Client with new Stripe(<API_KEY>)
  • Wrap API call in try/catch block
  • Call invoices.create method with an invalid payload (Such as the one in the snippet)
  • Call will not succeed, error, or finalize
  • Lambda execution will end

Expected behavior

When an API call fails on any non 2xx status, it should throw an exception that can be caught by a try/catch block or a .catch.

Code snippets

const StripeClient = new Stripe(env.STRIPE_BILLING_API_SECRET_KEY);

try {
  const stripeSubscription = await StripeClient.subscriptions.create({
    customer: 'no-existent-id',
    collection_method: 'send_invoice',
    currency: 'usd',
    days_until_due: 14,
    off_session: true,
    proration_behavior: 'none',
    payment_settings: {
      payment_method_types: ['us_bank_account'],
    },
  });

  console.log(stripeSubscription);
} catch (err) {
  console.log(err);
  throw err;
}

OS

Running on AWS Lambda node 20.x Runtime

@jar-stripe
Copy link
Contributor

@Marlrus thank you for the detailed explanation and updates here! Can you also share a self-contained example NestJS project that demonstrates the the behavior? And, do you know if this behavior occurs if you are not running in Docker (i.e. if you run a NestJS project on your personal computer or on a bare-metal server)?

@Marlrus
Copy link
Author

Marlrus commented Nov 20, 2024

Hi @jar-stripe I can't provide a self-contained NestJS project at the moment but I will be able to provide one in the following days.

I can confirm that this same behavior happens when not running the app on Docker as I ran the nest build and nest start steps to the same result.

I can also confirm that this happens on node version 18.x on NestJS Docker and Direct, as well as AWS Lambda.

Temporary Workaround

I have managed a temporary workaround that has allowed me to push through by using node-fetch with the node_modules/stripe/esm/net/FetchHttpClient.js module:

import fetch from 'node-fetch';

const httpClient = Stripe.createFetchHttpClient(fetch);
const StripeClient = new Stripe(env.STRIPE_BILLING_API_SECRET_KEY, { httpClient });

Based on my current TypeScript project config this will only work with npm i [email protected] as changing the config to fit the ERR_REQUIRE_ESM error breaks a lot of modules on the project. This is less than ideal as the version is old but I had to make the compromise to meet delivery.

New Insights and Follow Ups

Given that this fixes the issue, it appears that something with the way the SDK integrates with Node's http module is causing the issue. I had to dig heavily into the d.ts files and look at the modules to even find this as an alternative as the httpClient configuration option is not mentioned in any of the official docs.

Given more time I think I can create a cleaner workaround by writing an HTTP client using axios that mimics the node-fetch API so I can override the httpClient with something more powerful which should work given the info on the d.ts file:

    export const createFetchHttpClient: (
      /** When specified, interface should match the Web Fetch API function. */
      fetchFn?: Function
    ) => HttpClient<
      HttpClientResponse<
        /**
         * The response type cannot be specified without pulling in DOM types.
         * This corresponds to ReturnType<WindowOrWorkerGlobalScope['fetch']>
         * for applications which pull in DOM types.
         */
        // eslint-disable-next-line @typescript-eslint/no-explicit-any
        any,
        /**
         * The stream type cannot be specified without pulling in DOM types.
         * This corresponds to ReadableStream<Uint8Array> for applications which
         * pull in DOM types.
         */
        // eslint-disable-next-line @typescript-eslint/no-explicit-any
        any
      >
    >;

@Marlrus Marlrus changed the title Requests not Throwing Errors in NestJS App or AWS Lambda Node 20.x Requests not Throwing Errors in NestJS App or AWS Lambda Node 20/18.x Nov 20, 2024
@AntonioAngelino
Copy link

FYI: We have the same issue using React Remix, node 22 and Undici as the default library (i.e. https://remix.run/docs/en/main/guides/single-fetch enabled).

@jar-stripe
Copy link
Contributor

jar-stripe commented Nov 22, 2024

Thanks @Marlrus and @AntonioAngelino ! @Marlrus I'll take a look at what you have and try and reproduce it; if you (or @AntonioAngelino ) can send me a project that definitely has the issue that will short circuit this for sure. In either case, will update when I know more here!

Edit: @Marlrus sorry, I just noticed your New Insights. Thanks for running that to ground; would love to see what you come up with here (in addition to being able to reproduce this on our end!)

@jar-stripe jar-stripe self-assigned this Nov 22, 2024
@AntonioAngelino
Copy link

@jar-stripe I fixed using

import fetch from 'node-fetch';
const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!, {
  apiVersion: '2024-06-20',
  httpClient: Stripe.createFetchHttpClient(fetch)
});

Using node-fetch, the following code doesn't hang and throws an error as expected (the error was: "You passed an empty string for 'starting_after'. We assume empty values are an attempt to unset a parameter; however 'starting_after' cannot be unset. You should remove 'starting_after' from your request or supply a non-empty value.")

const invoicesResponse = await stripe.invoices.list({
            customer: props.customerId,
            starting_after: "",
            limit: props.limit ?? 10,
        });

In order to reproduce the bug, you can run

import { fetch } from 'undici';
const stripe = new Stripe(process.env.STRIPE_SECRET_KEY!, {
  apiVersion: '2024-06-20',
  httpClient: Stripe.createFetchHttpClient(fetch)
});
const invoicesResponse = await stripe.invoices.list({
            customer: props.customerId,
            starting_after: "",
            limit: props.limit ?? 10,
        });

The Stripe library should be compatible with the fetch implementation of undici ( https://github.com/nodejs/undici )

@jar-stripe
Copy link
Contributor

Thanks @AntonioAngelino ! We'll take a look.

@jar-stripe jar-stripe removed their assignment Dec 9, 2024
@jar-stripe jar-stripe self-assigned this Dec 17, 2024
@jar-stripe
Copy link
Contributor

jar-stripe commented Dec 17, 2024

A quick update from our end: I cannot reproduce the hanging behavior as described above. I tried to reproduce this with a brand new NestJS (latest version) app using the examples shared above on Node 18 and 20 running on my machine (no docker, no Lambda) and no luck. I have a sneaking suspicion there's some difference between our environments. If someone is able to share a Dockerfile that showcases the issue, I think that would be a good place to go next.

@Marlrus I'm not sure I follow what you are saying about .on methods not being available; what I am seeing from my side is that when toJSON is called, we attach the .on and .once handlers and then shortly after those handlers get called. I did notice response.removeAllListeners(); in your open-telemetry code. Is that running inside the same node application as the Stripe SDK?
What happens if you remove that line?

@AntonioAngelino
Copy link

@jar-stripe Did you plan to support undici?

@AntonioAngelino
Copy link

@jar-stripe About the AWS Lambda issue, I think the issue is related to nodejs/undici#3133 .

Since the native implementation of fetch is based on undici starting from Node 18, the stripe library should support undici to avoid any issue.

@jar-stripe
Copy link
Contributor

jar-stripe commented Dec 18, 2024

@AntonioAngelino as far as I know it hasn't come up explicitly but I can add an item into our back log to investigate! I did want to clarify though that I tried your examples using undici and I was not able to reproduce the issue. I tried this on stripe-node v16 (the SDK version that pins the API version you've specified) and stripe-node v17.3.0 (from the original issue) and did not have any issues getting an error response back. So I think I am still missing something about the failure mode here.

Re: your steps to reproduce, were you running that on AWS Lambda?

@AntonioAngelino
Copy link

@jar-stripe I'll create a codesandbox repo to reproduce it. We're using a Docker image using node:22-bookworm-slim

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

No branches or pull requests

3 participants