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

[Bug]: Undocumented breaking change - acknowledgment to handleMessage #443

Closed
exortech opened this issue Nov 23, 2023 · 7 comments · Fixed by #446
Closed

[Bug]: Undocumented breaking change - acknowledgment to handleMessage #443

exortech opened this issue Nov 23, 2023 · 7 comments · Fixed by #446

Comments

@exortech
Copy link

Describe the bug

This commit changed the behaviour of deleting messages after processing: 3896a48

It is now necessary to return the message from the handler function - or an object with a matching message id.

This change is undocumented. The main README still states:
"Messages are deleted from the queue once the handler function has completed successfully."

It does not specify that the message object needs to be returned from the handler.

For any users upgrading sqs-consumer unaware of this change will have messages stuck on the queue.

Your minimal, reproducible example

Self-evident

Steps to reproduce

Upgrade sqs-consumer from an earlier version

Expected behavior

At the very least this change should be documented. Ideally it would be highlighted as a breaking change for any upgrading users.

How often does this bug happen?

None

Screenshots or Videos

No response

Platform

Node v18

Package version

v7.4.0

AWS SDK version

No response

Additional context

No response

@nicholasgriffintn
Copy link
Member

This shouldn't be the case at all, I had a quick look at our tests and they appear to be testing a scenario in which a message id is not returned and the message is still deleted.

I'll have another proper look later, if it is the case, we'll get a release out to solve it.

@nicholasgriffintn
Copy link
Member

So it looks like the code will return a message for you if what you returned from message was not an object:

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

So it should be backwards compatible, as long as what you were previously returning is not an object.

@exortech
Copy link
Author

Thanks Nicholas. That's a big assumption. There's nothing in the prior behaviour that specified or depended on what could or couldn't be returned from a handler. Our code was returning the processed entity from the handler to support unit testing the handler function. That seems like perfectly reasonable behaviour.

Why would the code not either i) always return the message or ii) check to see if the returned object is a Message instead of an Object?

At the very least if this is an intended change in behaviour, it needs to be captured and documented as a breaking change.

@nicholasgriffintn
Copy link
Member

Yup that all makes sense, really it was probably overlooked, but we need to allow people to acknowledge no messages, so the check for if it's an object is allowing for that capability.

Something we can definitely look into further.

@nicholasgriffintn
Copy link
Member

nicholasgriffintn commented Nov 23, 2023

I think at the least, we'd need to document the feature better, in terms of the check to see if the object contains any ids, or always returning the ids, that would remove the ability to acknowledge no messages.

Possibly a feature to override this behaviour might work well, but we'd also need to consider that changing how this works may also result in a breaking change.

@nicholasgriffintn
Copy link
Member

Let me know your thoughts on the above API option ^ #446 .

We'll be looking at merging this in sometime next week.

Copy link

This issue has been closed for more than 30 days. If this issue is still occuring, please open a new issue with more recent context.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants