-
-
Notifications
You must be signed in to change notification settings - Fork 217
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
Failure/Error: raise ThreadError, 'queue empty' if @queue.empty? #198
Comments
I feel like there are race conditions but I couldn't reproduce them. That being said, here is what I got in at least one case:
Compared to a successful run:
My initial assessment is that the web request to get the |
Another odd thing, I've run a single integration test using |
Hmmm. So, I could only reproduce this issue when bumping up against rate limits. I feel like the
Here is the same spec, earlier, which was successful:
|
I noticed the first event is different: Failed
Succeeded
How can we get a closed event before the connection even started? Is there a race condition with previous spec? (it passed in both cases above). |
I think I see what's happening. The spec is flakey, but in a very odd way caused by threads.
Here is my theory:
What do you think? |
I don't think this is a bug in the actual code, but a problem with how the spec is executing. This class of bugs is avoided entirely by async, so perhaps the solution is to accept the buggy spec for 0.12.x, and the problem would be resolved in 0.13.x because threads wouldn't be used. |
@dblock ping :) |
If this is a race condition we should be able to wait for the thread in |
I think that's the right fix. You should definitely call |
Just FYI, that's why |
Had to revert the fix in 5effe17, needs changes for faye-websocket and celluloid.
|
I know it’s tricky but you definitely shouldn’t just leave threads hanging around. |
Yes @ioquatix, I'll fix it later. |
I am travelling for the next week but will be happy to review the code after that. |
Fix #198: Use a thread for Celluloid, shutdown EM.
Seems like a reproducible, but inconsistent failure in Travis.
https://travis-ci.org/slack-ruby/slack-ruby-client/jobs/332488852
The text was updated successfully, but these errors were encountered: