toadhopper
toadhopper copied to clipboard
toolmantim
open_timeout is too short
Currently, we're setting the open_timeout to 2 seconds and the read_timeout to 5. I've seen timeouts exceeded on occasion when the test suite posts to airbrake.io. In one particular case, the timeout was just a plain ol' http posting (not even the overhead of https).
I recommend we bump up the default open_timeout significantly, perhaps something like 8 seconds.
This "Issue" here is meant to just open up discussion for the change. Thoughts?
That particular test is causing me problems right now: http://pastie.org/4219078
It's really unfortunate that these requests don't come back really quickly. They should really be processed async. If there was a major error you could have X number of requests blocking for 8 seconds (if they were sending errors). It would be really awesome if there was a mechanism for "deferred" sending via a thread pool (or EM.defer).
I think there's not a huge difference between 2 and 8 seconds. I think defaulting to a conservative "make sure nothing gets dropped" timeout makes more sense than defaulting to a "drop some errors sometimes" timeout. If someone has business requirements that are different, they could then lower the timeout for that use case. So I would be comfortable with the default moving to 8 seconds here.
+1 to upping the default timeout.
I'm not sure we want to go with all the complexity of an EM defer queue. Yes, it is possible for exceptions to stack up, but hopefully that is rare (it should definitely not be the norm). It also adds a non-trivial dependency on EM, which not every project will have or want. I am in favor of keeping this library as simple/small as possible to reduce the chance of it raising exceptions while handling exceptions or failing to report any errors.
I didn't suggest that this gem use EM, I was suggesting that a mechanism for async (such as a thread pool) could be provided. That way you wouldn't have to hold the entire thread, which would also hang the response to the end user. You could also provide a mechanism for EM support without adding an EM dependency requirement, Sinatra does this in a few places: https://github.com/sinatra/sinatra/blob/master/lib/sinatra/base.rb#L116
That way, the default timeout could be much longer and it wouldn't have the consequence of shutting down a busy web application in the event the exception server starts locking up. Not a huge priority (or necessarily a proposal), just a thought.