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

update to new shutdown semantics #21

Closed
wants to merge 1 commit into from

Conversation

talevy
Copy link
Contributor

@talevy talevy commented Sep 17, 2015

@talevy
Copy link
Contributor Author

talevy commented Sep 17, 2015

not sure whether this line needs to be changed to while !stop? since the stop will close the socket anyways and render an exception to be raised.

https://github.com/logstash-plugins/logstash-input-syslog/pull/21/files?diff=unified#diff-820664ee3bf7ffecf60ddb6df23415f3L137

@@ -116,7 +115,7 @@ def run(output_queue)
def server(protocol, output_queue)
self.send("#{protocol}_listener", output_queue)
rescue => e
if @shutdown_requested.false?
if !stop?
@logger.warn("syslog listener died", :protocol => protocol, :address => "#{@host}:#{@port}", :exception => e, :backtrace => e.backtrace)
sleep(5)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might need to be updated to use Stud.stoppable_stop { stop? }, what do you think?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not using stoppable_stop here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated to include this (stoppable_sleep)

@purbon
Copy link

purbon commented Sep 18, 2015

@talevy RE: #21 (comment)

I don't think is necessary, however In some plugins I made I changed the infinite loop just to indicate better that this loop will stop some time.

@talevy
Copy link
Contributor Author

talevy commented Sep 18, 2015

@purbon updated with your comments! thanks!

@purbon
Copy link

purbon commented Sep 21, 2015

LGTM

@elasticsearch-bot
Copy link

Merged sucessfully into master!

@talevy talevy closed this in 1712508 Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants