bbc / sqs-consumer

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

feat: adding an abort method #352

Closed nicholasgriffintn closed 1 year ago

nicholasgriffintn commented 1 year ago

Resolves #346

Description:

sqs-consumer now provides some new functionality that also aborts existing requests when .stop() is called with the expected parameter.

To do this users to tell us to abort any requests to AWS SQS, like so:

consumer.stop({abort: true})

To support backwards compatibility, this option would default to false. To note, we are also setting this as a param for consumer.stop() rather than an option as I feel like some occasions you may want to abort while others you might not want to, so this allows us to support both cases.

If the option is true, we should implement and use the AbortController to abort SQS requests as discussed here:

https://aws.amazon.com/blogs/developer/abortcontroller-in-modular-aws-sdk-for-javascript/

Type of change:

Why is this change required?:

As described in the original issue (https://github.com/bbc/sqs-consumer/issues/346), some users expect that when they call stop, not only will sqs-consumer stop the polling requests but it will also abort any pre-existing requests that are in flight to SQS.

While this is being implemented as optional for this, this may be changed to required in a later release, should we feel that to be advantageous, it's likely that this may need some validation in the real world before then though.

Code changes:


Checklist:

nicholasgriffintn commented 1 year ago

Given that AbortController does not support Node 14 (I knew that but forgot prior to the tests failing...), we probably want to think about how we will implement this.

I don't think we want to remove support for Node 14 yet, but I also don't think that we want to create a complex solution for something that we definitely won't be supporting for much longer as Node 14 is only in maintenance until Apr 2023.

Should we want to remove support for Node 14, this would have to be part of a major release instead of a minor.

artur-ma commented 1 year ago

meybe u can use https://github.com/mysticatea/abort-controller on node14 example

https://github.com/nodejs/undici/blob/main/test/abort-controller.js#L10-L20

nicholasgriffintn commented 1 year ago

meybe u can use https://github.com/mysticatea/abort-controller on node14

example

https://github.com/nodejs/undici/blob/main/test/abort-controller.js#L10-L20

Thanks, the AWS SDK is another option if that works with 14 https://www.npmjs.com/package/@aws-sdk/abort-controller but ideally we wouldn't add another package to it.

The best course of action may just be to tackle the bugs that we have first and then release this as a major, so anyone on 14 still gets the bug fixes for the period that they're switching to 16.

codeclimate[bot] commented 1 year ago

Code Climate has analyzed commit 7fc12980 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 94.5% (0.2% change).

View more on Code Climate.

nicholasgriffintn commented 1 year ago

Code Climate has analyzed commit 5cbd791 and detected 2 issues on this pull request.

The issues here are for duplication of the options that are passed to SQS, this is something that I want to look into further to make sure that only having one AbortController is the correct way to go, I think it is, and if it is, we may just want to create an SQS Options variable that can be reused in these SDK calls.