smithy-lang / smithy-typescript

Smithy code generators for TypeScript. (in development)
Apache License 2.0
211 stars 78 forks source link

[BUG] Stale timeouts in node-http-handler #1305

Closed deftomat closed 1 month ago

deftomat commented 1 month ago

According to the node-http-handler source code, there may be 3000ms timeouts which are never canceled when the request fails.

Context:

We are having Lambdas which are calling DynamoDB with on-demand scaling in a large burst as users starts coming to the system in the morning and there is a possibility that Dynamo responds with the error and AWS-SDK v3 will automatically retry the request which will then success.

Due to this, a timeout created in node-http-handler for the first request (failed one) will never be canceled and our lambdas took 3 seconds to respond even when our own code + DynamoDB call took less than 25ms.

Lambdas, by default, do not respond until the event loop is empty. So, this 3 seconds timeout is increasing our response times from a few milliseconds to 3 seconds unnecessarily.

kuhe commented 1 month ago

Lambdas, by default, do not respond until the event loop is empty

How do you demonstrate this?

setTimeout(() => {
  console.log('after 3000');
}, 3000);
export const handler = async (event) => {
  setTimeout(() => {
    console.log('after 3000');
  }, 3000);
  const response = {
    statusCode: 200,
    body: JSON.stringify('Hello from Lambda!'),
  };
  return response;
};

This Lambda fn responds immediately because the handler function returns a value. The two timeouts in and outside the handler are not blocking.

deftomat commented 1 month ago

Well, It will wait for an empty event loop when you use callback (3rd handler parameter) instead of returning promise.

We have been forced to used callbacks instead of returning promises as otherwise, even the one missing await may lead to unsaved data. By using callbacks, you can avoid this problem. A lot of libraries for lambdas (apollo-server, etc.) are using callbacks instead of promises as you cannot trust your code without it. We had a LOT of problems in production with that, so we switched to callbacks which makes sure that there is no pending request to database, etc.

kuhe commented 1 month ago

I see, I'll put together a patch that clears the timeouts.

kuhe commented 1 month ago

The fix is available in https://www.npmjs.com/package/@smithy/node-http-handler/v/3.1.0, if you update your lockfiles and use a recent AWS SDK version that has an open range of ^3.x for that Smithy package.

We (AWS SDK for JavaScript v3) will also update a future version of the AWS SDK to include this fix as the minimum required version of the node-http-handler dependency.

deftomat commented 1 month ago

Awesome! Thanks for the fix.