search

voloko / twitter-stream

Twitter realtime API client
MIT License
233 stars 81 forks source link

Swallowing all Exceptions #28

Closed timhaines closed 12 years ago

timhaines commented 12 years ago

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 commented 12 years ago

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

rud commented 12 years ago

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?