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

UnhandledPromiseRejection when call to SQS deleteMessageBatch rejects #50

Closed
jmac105 opened this issue Sep 17, 2019 · 3 comments · Fixed by #51
Closed

UnhandledPromiseRejection when call to SQS deleteMessageBatch rejects #50

jmac105 opened this issue Sep 17, 2019 · 3 comments · Fixed by #51
Assignees
Labels
bug Something isn't working
Milestone

Comments

@jmac105
Copy link

jmac105 commented Sep 17, 2019

Your issue may already be reported!
Please search on the issue tracker before creating one.

Expected Behavior

If the call to deleteMessageBatch rejects, then that rejected Promise should be handled properly and not bubble up.

Current Behavior

An UnhandledPromiseRejectionWarning is logged to console, for example:
(node:58881) UnhandledPromiseRejectionWarning: AWS.SimpleQueueService.NonExistentQueue: The specified queue does not exist for this wsdl version.

Possible Solution

The cause of this bug is that a rejected Promise is returned from https://github.com/PruvoNet/squiss-ts/blob/master/src/Squiss.ts#L352, however that rejected Promise is not caught at https://github.com/PruvoNet/squiss-ts/blob/master/src/Squiss.ts#L471 which is where the call to _deleteMessages occurs.

Proposed solutions:

  1. Return resolved promise from catch block in _deleteMessages, the error has already been emitted so no need to reject as well?
  2. Add .catch() handler in _deleteXMessages which suppresses the error

Steps to Reproduce (for bugs)

Can be replicated via the unit tests. For visibility set https://github.com/PruvoNet/squiss-ts/blob/master/src/test/src/index.spec.ts#L725 to be the only test to run:
it.only('emits error when delete call fails', () => {
Warnings can then be seen in the console

[me@mymachine squiss-ts (master)]$ npm test

> [email protected] test /Users/me/GitHub/squiss-ts
> npm run lint && npm run mocha


> [email protected] lint /Users/me/GitHub/squiss-ts
> tslint -c tslint.json 'src/**/*.ts' 'test/**/*.ts'


> [email protected] mocha /Users/me/GitHub/squiss-ts
> mocha --opts src/test/mocha.opts



  index
    Failures
(node:70693) UnhandledPromiseRejectionWarning: Error: test
    at Object.promise (/Users/me/GitHub/squiss-ts/src/test/src/index.spec.ts:731:41)
    at getQueueUrl.then (/Users/me/GitHub/squiss-ts/src/Squiss.ts:348:16)
    at <anonymous>
(node:70693) 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(). (rejection id: 3)
(node:70693) [DEP0018] DeprecationWarning: Unhandled promise rejections are deprecated. In the future, promise rejections that are not handled will terminate the Node.js process with a non-zero exit code.
      ✓ emits error when delete call fails (72ms)


  1 passing (84ms)

Context

We are currently testing out squiss-ts by testing it in different scenarios before implementing in production code. A real world example which would cause an error like this would be a network blip.

Your Environment

squiss-ts: 4.0.6
Node: 8.16.1

@jmac105 jmac105 added the bug Something isn't working label Sep 17, 2019
@regevbr
Copy link
Contributor

regevbr commented Sep 17, 2019

@jmac105 thanks for reporting the issue! I will check it right away :-)

@regevbr regevbr added this to the v4.0.7 milestone Sep 17, 2019
regevbr added a commit that referenced this issue Sep 17, 2019
… reject (#51)

* Fix #50 UnhandledPromiseRejection when call to SQS deleteMessageBatch rejects
regevbr added a commit that referenced this issue Sep 17, 2019
@regevbr
Copy link
Contributor

regevbr commented Sep 17, 2019

@jmac105 the issue is fixed and released in 4.0.7
Thanks again for reporting!

@jmac105
Copy link
Author

jmac105 commented Sep 17, 2019

Fantastic response time, thanks @regevbr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants