bbc / sqs-consumer

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

feat: Wait for msgs to be processed before emitting stopped #454

Closed barwin closed 5 months ago

barwin commented 5 months ago

Resolves https://github.com/bbc/sqs-consumer/discussions/410

Description:

Optionally delay the emit('stopped') event until in-flight messages from the most recent poll have been processed.

Thank you for creating and maintaining a great module. sqs-consumer has been pleasant tool to work with.

I'm entirely open to other suggestions of how to address the problem. Maybe I'm missing something that already exists to accomplish this. Thanks in advance.

Type of change:

Why is this change required?:

We are implementing a graceful shutdown of queue consumers just before issuing a process.exit() in our app

I'd like to support waiting for in-flight messages to be finalized before exiting, but it seems the current implementation emits stopped without that guarantee.

Code changes:


Checklist:

barwin commented 5 months ago

~With fresh eyes, this implementation has some flaws I want to work out before review. I also now see there is a draft PR with a concept of polling status, which would be useful for this feature ..~

updates pushed

nicholasgriffintn commented 5 months ago

Notes:

With a similar check for stopping to this: https://github.com/bbc/sqs-consumer/blob/main/src/consumer.ts#L126

Then we might want to adjust this function to return all three states:

https://github.com/bbc/sqs-consumer/blob/main/src/consumer.ts#L151

(This comment would make it breaking though, so maybe for the future...)

nicholasgriffintn commented 5 months ago

Thanks for this PR btw! Looking good!

github-actions[bot] commented 5 months ago

CLA Assistant Lite bot CLA CHECK All Contributors have signed the CLA

codeclimate[bot] commented 5 months ago

Code Climate has analyzed commit bbfb1cc1 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 98.3% (0.0% change).

View more on Code Climate.

barwin commented 5 months ago

I have read the CLA Document and I hereby sign the CLA

barwin commented 5 months ago

Thanks for the feedback, @nicholasgriffintn - I've pushed changes that I think address all the requests, but left one clarification question about the date stuff.

I like your suggestion in https://github.com/bbc/sqs-consumer/pull/454#issuecomment-1913423613 but I agree it may be nice as a follow up for later since it would be a breaking change

nicholasgriffintn commented 5 months ago

This is looking good btw, sorry for the delay, it's on my list :).

barwin commented 5 months ago

This is looking good btw, sorry for the delay, it's on my list :).

All good, thanks!