search

bbc / sqs-consumer

Build Amazon Simple Queue Service (SQS) based applications without the boilerplate
https://bbc.github.io/sqs-consumer/
Other
1.74k stars 333 forks source link

Heartbeat errors are not propagated / catched #275

Closed michaelwittig closed 2 years ago

michaelwittig commented 3 years ago

Describe the bug If the call to changeVisabilityTimeout fails, the Promise is rejected but no one notices. Node.js complains about this with an 

UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)

To Reproduce Steps to reproduce the behaviour:

  1. I was running into this because of a missing IAM permission #274 and I think that is the easiest way to reproduce the issue

Expected behaviour I'm not sure but it could be an error event?

I'm willing to provide a fix if we can agree on what's expected.

rafael-pb commented 2 years ago

I believe I had a similar issue in my application. The stack trace didn't help, but my application got killed due to unhandled exception/rejection (my app is configured to die if such thing happen). The reason was changeMessageVisibility call that failed.

By inspecting the code of sqs-consumer, I believe the reason is that inside the methods changeVisabilityTimeoutBatch and changeVisabilityTimeout they do not "await" the promise; therefore the try {} catch {} does not catch the error. It also happens because those who call it do not catch any errors (one is inside a timer and the other is inside a catch block).

I believe adding the await should fix the issue.

michaelwittig commented 2 years ago

the issue is fixed. Thanks @rafael-pb