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

Swallowing all Exceptions #28

Closed
timhaines opened this issue May 2, 2012 · 2 comments · Fixed by #30
Closed

Swallowing all Exceptions #28

timhaines opened this issue May 2, 2012 · 2 comments · Fixed by #30

Comments

@timhaines
Copy link

There's a rescue Exception => ex

here:
https://github.com/voloko/twitter-stream/blob/master/lib/twitter/json_stream.rb#L211

Why this is bad: http://www.mikeperham.com/2012/03/03/the-perils-of-rescue-exception

@rud
Copy link
Contributor

rud commented May 2, 2012

@timhaines an excellent point, I agree it's a problem. Would you be so kind as to demonstrate the issue with a spec + patch?

jacegu added a commit to jacegu/twitter-stream that referenced this issue May 4, 2012
@rud
Copy link
Contributor

rud commented May 7, 2012

I agree something has to change, unfortunately I'm not sure we're there yet. The blog-post referenced mentions simply rescuing StandardError (default) vs. explicitly catching Exception, an important distinction.

I'd say the appropriate behavior would be for receive_stream_data to rescue StandardError, notifies any error-callbacks (as happens now), closes the stream (allowing a reconnect to happen), and then keep swallowing the StandardError exception. This is to allow the current reconnect semantics to function. receive_stream_data is used as the on_body callback to Http::Parser, which has it's own set of expectations on appropriate return values.

Structured like that, SystemExit, Interrupt and friends can now propagate out correctly (not caught), without causing an automatic reconnection. What happens next depends on how your EventMachine instance is configured, correct?

Alternate fix in my commit 329dba6, any thoughts on that?

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 a pull request may close this issue.

2 participants