Skip to content

SQS: Properly handle Errors in listeners #1383

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

SQS: Properly handle Errors in listeners #1383

wants to merge 1 commit into from

Conversation

isaacvando
Copy link

@isaacvando isaacvando commented May 8, 2025 •
edited
Loading

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

I updated the listener adapters to catch Throwable instead of Exception so that ErrorHandlers can handle Throwables as expected.

💡 Motivation and Context

Fixes #1379.

💚 How did you test it?

I added unit tests to the classes I modified and tested the change locally within my own project against the reproduction in the issue.

I would be happy to add an integration test if appropriate. If so could you give me some direction about your preferences for that?

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • I updated reference documentation to reflect the change
  • All tests passing
  • No breaking changes

🔮 Next steps

@github-actions github-actions bot added the component: sqs SQS integration related issue label May 8, 2025
@isaacvando isaacvando marked this pull request as ready for review May 8, 2025 20:57
@tomazfernandes
Copy link
Contributor

Hey @isaacvando, thanks for the PR!

I would be happy to add an integration test if appropriate. If so could you give me some direction about your preferences for that?

I'm happy with the tests you added, so unless you'd like to test an specific flow with an integration test, I don't think we'd need specific tests to assert correct handling for Error. If you do think there's a flow you'd like to test though, feel free to.

On a quick search on the codebase for catch(Exception I saw there's quite a few other places in SQS (and other integrations) where we have this problem, so if you would like to pick up a few more on SQS side that would be really helpful.

While Errors are rarely thrown, I've recently had just that situation with an AWS Glue deserializer and lost a few precious hours to understand what the issue was, so if we can avoid our users from going through that pain, it'd be better.

Totally up to you though, happy to merge this PR as is and open a new issue for that.

Please let me know your thoughts, thanks.

@isaacvando
Copy link
Author

Great, @tomazfernandes! I will be out of office for a while so I think it would be best to go ahead and merge the PR now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maciejwalkowiak maciejwalkowiak Awaiting requested review from maciejwalkowiak maciejwalkowiak is a code owner

@MatejNedic MatejNedic Awaiting requested review from MatejNedic MatejNedic is a code owner

@tomazfernandes tomazfernandes Awaiting requested review from tomazfernandes tomazfernandes is a code owner

Assignees
No one assigned
Labels
component: sqs SQS integration related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Errors are not handled when thrown in an SQS listener
2 participants
@isaacvando @tomazfernandes