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

Fix thread and memory leak on pipeline reload. #256

Closed
wants to merge 1 commit into from

Conversation

jakelandis
Copy link
Contributor

@jakelandis jakelandis commented Dec 14, 2017

When a pipeline is reloaded, the rufus stop/shutdown method is called without any arguments. This instructs rufus to leave it's worker threads running. To fully shutdown the rufus worker threads, a :wait or :kill is needed. This change sends the :wait parameter to the shutdown method to ensure that the work thread is stopped. Prior to this change, the thread and all associated memory would stay in the JVM indefinitely.

Fixes #255

When a pipeline is reloaded, the rufus stop/shutdown method is called without any arguments. This instructs rufus to leave it's worker threads running. To fully shutdown the rufus worker threads, a :wait or :kill is needed. This change sends the :wait parameter to the shutdown method to ensure that the work thread is stopped. Prior to this change, the thread and all associated memory would stay in the JVM indefinitely.

Fixes logstash-plugins#255
@elasticsearch-bot elasticsearch-bot self-assigned this Dec 14, 2017
@jakelandis
Copy link
Contributor Author

Note to reviewer : stop is an alias to shutdown and the code there is pretty clear on what it does with the options.

I also have evidence of fix on #255

Copy link
Contributor

@yaauie yaauie left a comment

Choose a reason for hiding this comment

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

LGTM.

Good find.

Why on earth they'd let you shutdown without that argument makes no sense at all to me, especially since there is no facility to "resume" after a shutdown is initiated.

@jakelandis
Copy link
Contributor Author

@yaauie - thanks

@elasticsearch-bot
Copy link

Jake Landis merged this into the following branches!

Branch Commits
master 4230f04

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

Successfully merging this pull request may close these issues.

3 participants