-
Notifications
You must be signed in to change notification settings - Fork 119
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
Add HTTP default_timeout #78
Conversation
👍 |
hey @dramalho, great work. i also think that a default timeout is a good idea for a library that handles HTTP requests on your behalf. in fact, we handled a similar feature request in the node module recently. i wanted to point that out because i did end up implementing a test for the functionality. the PR: https://github.com/opentok/opentok-node/pull/57/files. nock has a can i ask why you think 1 second is a good default? i chose based on the defaults browsers use. i found that firefox waits 5 minutes. assuming a server needs to make a handful of remote requests to be able to respond to a browser within those 5 minutes, i think its fair to divide by a reasonable number and come up with a timeout. i chose to divide by 25 and arrived at a 20 second default timeout. any thoughts? |
Ahh yeah, sorry, in retrospect 1s was a bit too aggressive and while I do get you 20s rule, I still rather be more aggressive here as this is server side and for synchronous calls it could potentially hold up clients for no discerning reason. So in saying that, this actually sets a couple of timeouts with the same value, connect and read for the connection adapter HTTParty uses, which I think by default is Net::HTTP. On that, Connect has no default timeout and really, I wouldn't expect a socket to take 20 seconds to open under normal circumstances and Read actually has a 60 second timeout, but - if I read correctly - the timeout applies to reading packets - blocks? - so we don't expect 60 second gaps between the multiple parts of a normal http reply. So, Yes, I'll increase this value, I'm actually ok with going with the 20 second rule - and it's your project guys, I just wanted to put a timeout in :) . I might want to be a bit more aggressive, but 20s is fine and if that's a value you already use, I'm ok with that. re: specs, yes, Webmock allows us to explicitly send a timeout, but AFAIK it only triggers the exception, while without this setting we may never get an exception at all :) - so I'm really not sure how to spec this out. btw, thanks for the quick reply @aoberoi , really appreciate you guys being active on this :) |
Sync with official repo
yikes, sorry for leaving this PR hanging for a little while. right after that praise too... i hadn't thought about the difference between the connection timeout and the actual response body timeout. it sounds like HTTParty isn't thinking too hard about it either. Neither is WebMock. i follow your reasoning on wanting to be more aggressive about the connection timeout, so i think 1s is a good value. i'd be happy to merge that change in. in terms of testing, yeah i get that WebMock just throwing the exception, so its just a simulation. but i still think its worth demonstrating that a timeout error is properly addressed by wrapping it in an |
:) No problem at all, I'll push some specs in. Incidentally, we've been running this off our fork and although it's extremely rare (<10 occurrences in the past 2 months) every now and then we do get a connection timeout exception and I'm very happy for the time out :) I'll write some specs later on then, thanks |
Added spec for `create_session` and `archives.all`
So, a couple of things up for discussion:
|
+1 |
👍 from me too |
hey great :) |
Simply adds a default timeout (connect / read) to
HTTParty
to ensure clients don't get unexpected delays in case of network issues.I do think it's a very rare event, but given:
I though I would just add the change and open a PR with you guys.
I'm not sure how to spec this out though, I could mock timeouts but that's probably not exactly what I want as it would just mock the exception and not the actual waiting period.
Thoughts from you guys?