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

SQSMessageConsumer doesn't handle errors gracefully #26

Open
paoloc0 opened this issue Dec 5, 2019 · 4 comments
Open

SQSMessageConsumer doesn't handle errors gracefully #26

paoloc0 opened this issue Dec 5, 2019 · 4 comments

Comments

@paoloc0
Copy link

paoloc0 commented Dec 5, 2019

Hi

SQSMessageConsumer has a tight loop in poll(), summarised as:

for (;;) {
   try {
      // receive a message
   } catch (Exception e) {
      exceptionHandler.accept(e);
   }
}

When an exception occurs (for example, kill network connectivity for a java.net.UnknownHostException: sqs.eu-west-1.amazonaws.com), this spins away and generates a huge stack trace every few milliseconds, and consumes all CPU resources very quickly.

Please consider sleeping a second upon exception.

Thanks

@paoloc0
Copy link
Author

paoloc0 commented Dec 5, 2019

I see that I can override the default exception handler via SQSMessageConsumerBuilder's withExceptionHandler. But having a sleep state in SQSQueueUtils.DEFAULT_EXCEPTION_HANDLER would still be a safer default. For instance, there seems to be the same problem in AmazonSQSRequesterClient, and I don't see a way to set an exceptionHandler for it to override the default one (I guess that would be a related feature request).

@robin-aws
Copy link
Contributor

Definitely not a bad idea to handle this better. There's already a sleep in place for QueueNotFoundExceptions for a similar reason. :) https://github.com/awslabs/amazon-sqs-java-temporary-queues-client/blob/master/src/main/java/com/amazonaws/services/sqs/util/SQSMessageConsumer.java#L126

That's a little more straight-forward, though, since that exception means it's very unlikely that immediately retrying will succeed. I wouldn't want to assume that about all exceptions, so I'm nervous about introducing an unconditional sleep. Perhaps we could hook up a configurable RetryPolicy? https://docs.aws.amazon.com/AWSJavaSDK/latest/javadoc/com/amazonaws/retry/RetryPolicy.html

@paoloc0
Copy link
Author

paoloc0 commented Dec 5, 2019

Whatever the mechanism, I think the most important is that the error handling is in the library user's control, which it is for consumers, but not for requesters. If I can override the default, I guess it wouldn't matter to me what the default behaviour was. (But yes, a RetryPolicy does look like a nice way to go.)

@yzhang-okta
Copy link

Hi, anyone fixing this ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants