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

Clean finished threads in Group::Spawner. #678

Merged

Conversation

marshall-lee
Copy link
Contributor

This is my attempt to fix #670

@digitalextremist
Copy link
Member

I will review this and test it. Thanks for the PR here.

@digitalextremist
Copy link
Member

So far I am very happy you patched this, and like what I see. My only concerns are to make sure the test is sufficient to cover all scenarios, which on first glance it seems to be... and wondering if the cleanup at shutdown ought to happen inside the synchronize block or not. I will merge this and make any needed tweaks but otherwise the behavior seems correct.

digitalextremist pushed a commit that referenced this pull request Sep 21, 2015
Clean finished threads in Group::Spawner.
@digitalextremist digitalextremist merged commit 0db078d into celluloid:master Sep 21, 2015
@marshall-lee
Copy link
Contributor Author

Some comments on why I had to change shutdown code exactly this way:

  • removed Queue usage in favor of simple array because there's no inter-thread communication in which Queue is helpful.
  • extracting join call out of synchronize there were two reasons: 1) there's simply no need in this since @mutex is aimed to guard only @group destruction. 2) since i used synchronize inside thread too there's a possible deadlock calling join on this thread inside of another synchronize — and even some tests with sleep were failing until I resolved this deadlock.

@digitalextremist
Copy link
Member

I thought you might be avoiding mutex deadlocking, understood.

@digitalextremist
Copy link
Member

There is a race condition in the test, on rbx

marshall-lee added a commit to marshall-lee/celluloid that referenced this pull request Sep 21, 2015
@marshall-lee
Copy link
Contributor Author

@digitalextremist I can't reproduce it on my computer but I understand how it's possible. Attempted to fix it in #679
Sorry for inattention, I should had write this spec just like the rest in spec/shared/group_examples.rb.

@digitalextremist
Copy link
Member

Your #679 PR is perfect. Thank you

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.

Memory Leak using Celluloid::Future
2 participants