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

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

Merged
merged 8 commits into from
Feb 6, 2024

Conversation

barwin
Copy link
Contributor

@barwin barwin commented Jan 23, 2024

Resolves #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:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to 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:

  • Add optional pollingCompleteWaitTimeMs option for Consumer.create() to enable the waiting behavior on shutdown.
  • Add method Consumer.waitForPollingToComplete() that loops with 1s delay until all messages are processed, when feature is enabled. The current Consumer.stop() method would use this to facilitate the waiting prior to emitting stopped

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@barwin barwin requested review from a team as code owners January 23, 2024 19:28
@barwin barwin marked this pull request as draft January 24, 2024 17:46
@barwin
Copy link
Contributor Author

barwin commented Jan 24, 2024

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

@barwin barwin marked this pull request as ready for review January 24, 2024 20:40
src/consumer.ts Outdated Show resolved Hide resolved
src/consumer.ts Outdated Show resolved Hide resolved
src/consumer.ts Outdated Show resolved Hide resolved
src/consumer.ts Outdated Show resolved Hide resolved
@nicholasgriffintn
Copy link
Member

nicholasgriffintn commented Jan 28, 2024

Notes:

  • I wonder what people's thoughts might be on if abort should also wait for polling to complete? Side thought mainly, possibly one for a discussion, just a note :).

  • We may also need to move this line: https://github.com/bbc/sqs-consumer/blob/main/src/consumer.ts#L132 to when it has actually stopped, and maybe add a this.stopping state in instead.

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
Copy link
Member

Thanks for this PR btw! Looking good!

Copy link

github-actions bot commented Jan 28, 2024

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

Copy link

codeclimate bot commented Jan 28, 2024

Code Climate has analyzed commit bbfb1cc 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.

@nicholasgriffintn nicholasgriffintn added this to the 8.2.0 milestone Jan 28, 2024
@barwin
Copy link
Contributor Author

barwin commented Jan 28, 2024

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

src/consumer.ts Outdated Show resolved Hide resolved
@barwin
Copy link
Contributor Author

barwin commented Jan 28, 2024

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 #454 (comment) but I agree it may be nice as a follow up for later since it would be a breaking change

@nicholasgriffintn
Copy link
Member

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

@barwin
Copy link
Contributor Author

barwin commented Feb 1, 2024

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

All good, thanks!

@nicholasgriffintn nicholasgriffintn merged commit 0fa5305 into bbc:main Feb 6, 2024
8 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants