Skip to content

Don't rescue ThreadError in FileThreadPool #56

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

Merged
merged 1 commit into from
Dec 18, 2015

Conversation

wfleming
Copy link
Contributor

The previous implementation was blindly catching all ThreadError
exceptions, because that's what #pop(true) raises when the queue is
empty. However, there are other situations that would raise the same
class that we definitely shouldn't be silently ignoring.

This changes the code to actually check if the queue is empty before
popping, and removes the rescue entirely: if exceptions happen, we want
to know.

👀 @codeclimate/review

yield item
end
rescue ThreadError
while !queue.empty? && item = queue.pop(true)
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think of using #any? here? If CC were running on this repo, I think it would recommend wrapping the assignment.

while queue.any? && (item = queue.pop(true))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The parens are a very good point. But there is no #any? defined on Queue[http://ruby-doc.org/core-2.2.3/Queue.html].

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so it doesn't. Thought it might mixin Enumerable, but it doesn't.

@dblandin
Copy link
Contributor

Just a couple nits. Looks good 👍

The previous implementation was blindly catching all ThreadError
exceptions, because that's what `#pop(true)` raises when the queue is
empty. However, there are other situations that would raise the same
class that we definitely shouldn't be silently ignoring.

This changes the code to actually check if the queue is empty before
popping, and removes the rescue entirely: if exceptions happen, we want
to know.
@wfleming wfleming force-pushed the will/dont-rescue-thread-error branch from 6c8c9f5 to 6e6ece7 Compare December 17, 2015 22:55
wfleming added a commit that referenced this pull request Dec 18, 2015
Don't rescue ThreadError in FileThreadPool
@wfleming wfleming merged commit 90b5a4e into master Dec 18, 2015
@wfleming wfleming deleted the will/dont-rescue-thread-error branch December 18, 2015 15:25
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