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

node-http-handler: probable regression for requestTimeout #4618

Closed
3 tasks done
raymondfeng opened this issue Apr 5, 2023 · 5 comments
Closed
3 tasks done

node-http-handler: probable regression for requestTimeout #4618

raymondfeng opened this issue Apr 5, 2023 · 5 comments
Assignees
Labels
bug This issue is a bug. investigating Issue is being investigated and/or work is in progress to resolve the issue. p3 This is a minor priority issue

Comments

@raymondfeng
Copy link

Checkboxes for prior research

Describe the bug

The following commit 86a6046 changes the behavior of node-http-handler, used by SQS client:

ReceiveMessageCommandInput https://sqs.us-west-1.amazonaws.com/220623082201/collabland-jobs-test: {
   MessageAttributeNames: [ '*' ],
   WaitTimeSeconds: 20,
   MaxNumberOfMessages: 10,
   VisibilityTimeout: 300,
   QueueUrl: 'https://sqs.us-west-1.amazonaws.com/220623082201/collabland-jobs-test'
 } +2ms

 Fail to receive/delete messages from https://sqs.us-west-1.amazonaws.com/220623082201/collabland-jobs-test: Error [TimeoutError]: Connection timed out after 5000 ms
     at ClientRequest.<anonymous> (/Users/rfeng/Projects/collabland/collabland-monorepo/node_modules/@aws-sdk/node-http-handler/dist-cjs/node-http-handler.js:91:38)
     at Object.onceWrapper (node:events:627:28)
     at ClientRequest.emit (node:events:513:28)
     at TLSSocket.emitRequestTimeout (node:_http_client:838:9)
     at Object.onceWrapper (node:events:627:28)
     at TLSSocket.emit (node:events:525:35)
     at Socket._onTimeout (node:net:562:8)
     at listOnTimeout (node:internal/timers:564:17)
     at process.processTimers (node:internal/timers:507:7) {
   '$metadata': { attempts: 1, totalRetryDelay: 0 }
 } +5s

SDK version number

"@aws-sdk/node-http-handler": "^3.306.0"

Which JavaScript Runtime is this issue in?

Node.js

Details of the browser/Node.js/ReactNative version

18

Reproduction Steps

  • Use AWS SDK to receive messages from a queue that does not have any messages

Observed Behavior

  • An error is thrown:
[TimeoutError]: Connection timed out after 5000 ms
     at ClientRequest.<anonymous> (/Users/rfeng/Projects/collabland/collabland-monorepo/node_modules/@aws-sdk/node-http-handler/dist-cjs/node-http-handler.js:91:38)
     at Object.onceWrapper (node:events:627:28)
     at ClientRequest.emit (node:events:513:28)
     at TLSSocket.emitRequestTimeout (node:_http_client:838:9)
     at Object.onceWrapper (node:events:627:28)
     at TLSSocket.emit (node:events:525:35)
     at Socket._onTimeout (node:net:562:8)
     at listOnTimeout (node:internal/timers:564:17)
     at process.processTimers (node:internal/timers:507:7) {
   '$metadata': { attempts: 1, totalRetryDelay: 0 }
 }

Expected Behavior

It should return empty message array. The connectionTimeout/requestTimeout should be used to control connection, not the http response.

Possible Solution

Workaround - increase requestTimeout to 30s

Additional Information/Context

No response

@raymondfeng raymondfeng added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Apr 5, 2023
@yenfryherrerafeliz yenfryherrerafeliz self-assigned this Apr 10, 2023
@yenfryherrerafeliz
Copy link
Contributor

Hi @raymondfeng, sorry to hear about your issues. I feel this is the expected behavior because, the requestTimeout parameter is designed to control how long a request should wait for it to complete, and in this case, since the time set for waiting for a new message is longer than the timeout set for the request to complete it indeed will throw a timeout error. However, I will bring this into discussion with the team to analyze this better.

Thanks!

@yenfryherrerafeliz yenfryherrerafeliz added investigating Issue is being investigated and/or work is in progress to resolve the issue. p3 This is a minor priority issue and removed needs-triage This issue or PR still needs to be triaged. labels Apr 10, 2023
@raymondfeng
Copy link
Author

I don't think it's by design, at least it's a breaking change. The docs says requestTimeout is for connection, not reading messages. Throws an error in polling messages is not desired.

@trivikr
Copy link
Member

trivikr commented Apr 26, 2023

The fix to undeprecate connectionTimeout and reintroduce functionality was pushed in PR #4669
It'll be released with v3.322.0 around 1pm Pacific either on Thu 04/27 or Fri 04/28

In the meantime, please use <= v3.295.0 as a workaround.

@trivikr
Copy link
Member

trivikr commented Apr 27, 2023

The fix was released in https://github.com/aws/aws-sdk-js-v3/releases/tag/v3.321.1

@trivikr trivikr closed this as completed Apr 27, 2023
@github-actions
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. investigating Issue is being investigated and/or work is in progress to resolve the issue. p3 This is a minor priority issue
Projects
None yet
Development

No branches or pull requests

3 participants