stripe / stripe-node

Node.js library for the Stripe API.
https://stripe.com
MIT License
3.89k stars 753 forks source link

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

Open Marlrus opened 3 days ago

Marlrus commented 3 days ago

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:

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 commented 3 days ago

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

I looked at the following comment (https://github.com/stripe/stripe-node/issues/1846#issuecomment-1638595957) 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:

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

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:

{
  "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 commented 2 days ago

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

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 commented 2 days ago

@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 commented 2 days ago

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 node-fetch@2.6.1 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
      >
    >;
AntonioAngelino commented 4 hours ago

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 commented 3 hours ago

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