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 memory leak caused by async tracking busy threads #289

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

mariokostelac
Copy link
Contributor

Some printf debugging foo and reading the code helped to figure out that we were sporadically leaking Thread objects when processors died with an error.
https://github.com/phstc/shoryuken/blob/e83a68bf9d460fdd368a38208032074cde085ca2/lib/shoryuken/processor.rb#L15 calls real_thread (which pairs Thread and Processor in manager's namespace) asynchronously and it sometimes executes before processor_died is executed.

I've set up logs for every addition/removal of Manager.threads elements and state after the action. Worker I've used failed randomly, approx 33% of times.

Logs:

A: ADD 70141427912260 <-- worker starts
A: KEYS [70141427912260]
A: DONE 70141427912260 <-- worker done
A: DELETE 70141427912260 <-- thread deletion command
A: KEYS [] <-- thread deleted
A: ADD 70141427912260
A: KEYS [70141427912260]
A: DONE 70141427912260
A: DELETE 70141427912260
A: KEYS [] <-- threads empty
A: DIED 70141427912260 <-- worker died
A: DELETE 70141427912260 <-- delete Thread! but hey, it's not even added
A: DELETE 70141427912260 FAIL!
A: KEYS []
A: ADD 70141427912260 <-- oh ok, we're adding it here?! it will stay in forever
A: KEYS [70141427912260]
A: ADD 70141422012480
A: KEYS [70141427912260, 70141422012480]

Fixing #88.

One +1 for simplifying the core and getting rid of asynchronous stuff where it's not needed. We should be more deliberate in defining thing as actors and establishing async communication between them.

@phstc phstc merged commit 453d6de into ruby-shoryuken:master Dec 14, 2016
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.

2 participants