logstash-plugins / logstash-input-sqs

Apache License 2.0
16 stars 40 forks source link

Plugin won't shutdown if no events coming through the SQS queue #33

Closed gregsterin closed 2 years ago

gregsterin commented 7 years ago

I've noticed that if an SQS queue has no events flowing through it, then the plugin won't shut down.

The use case I have for a queue without events flowing through it is that I use SQS as a fallback transport, and only send data through it when my primary transport is down or having issues streaming.

Looking at the code, it seems that breaking out of the polling loop happens only when there are messages to loop through: https://github.com/logstash-plugins/logstash-input-sqs/blob/master/lib/logstash/inputs/sqs.rb#L143

seems there needs to be a before_request callback (http://docs.aws.amazon.com/sdkforruby/api/Aws/SQS/QueuePoller.html#before_request-instance_method) that checks for stopped and throws ':stop_polling' to break out of the processing loop even if there are no events flowing through the queue.

danielpops commented 7 years ago

+1. I've been trying to track down why logstash was unable to shut down gracefully certain hosts. Turns out that this was the cause of it, since on those hosts in question, they have an SQS queue input that rarely receives input.

I can confirm that applying a patch like the one below addresses the problem

@@ -147,6 +147,10 @@
   def run(output_queue)
     @logger.debug("Polling SQS queue", :polling_options => polling_options)

+    poller.before_request do |stats|
+        throw :stop_polling if stop?
+    end
+
     run_with_backoff do
       poller.poll(polling_options) do |messages, stats|
         break if stop?
dustin-decker commented 6 years ago

This is causing noisy logs for us before our logstash containers hit their SIGTERM timeout of 5 minutes and get SIGKILLed. It'd be nice to get a fix merged.

jayqi commented 4 years ago

This still appears to be a problem in Logstash 7.9.1 and logstash-input-sqs 3.1.2.

SebastiaanDewilde commented 3 years ago

Still a problem, is it possible to merge the PR?