Permalink
Browse files

don't retry on failure when the request is a POST

  • Loading branch information...
andrewmbenton committed Aug 8, 2012
1 parent 60da7bb commit cc81aa5d066ef1a0d7badeddbe3f57b40211d561
Showing with 1 addition and 0 deletions.
  1. +1 −0 lib/twilio-ruby/rest/client.rb
@@ -216,6 +216,7 @@ def connect_and_send(request) # :doc:
raise Twilio::REST::ServerError
end
rescue Exception
+ raise if request.class == Net::HTTP::Post

This comment has been minimized.

Show comment Hide comment
@robink

robink Apr 22, 2013

why?

This comment has been minimized.

Show comment Hide comment
@kevinburke

kevinburke Apr 22, 2013

Contributor

Generally retrying a POST is not safe and could result in you purchasing a phone number, or sending the same message, multiple times. GETs on the other hand are not side-effecting and can be safely retried.

@kevinburke

kevinburke Apr 22, 2013

Contributor

Generally retrying a POST is not safe and could result in you purchasing a phone number, or sending the same message, multiple times. GETs on the other hand are not side-effecting and can be safely retried.

This comment has been minimized.

Show comment Hide comment
@andrewmbenton

andrewmbenton Apr 22, 2013

Contributor

Yes, the key word here is "idempotent." HTTP methods GET, PUT and DELETE are idempotent, while POST is not. It is safe to retry idempotent methods since the server state should be the same after trying a request n times as it would be after trying just once.

@andrewmbenton

andrewmbenton Apr 22, 2013

Contributor

Yes, the key word here is "idempotent." HTTP methods GET, PUT and DELETE are idempotent, while POST is not. It is safe to retry idempotent methods since the server state should be the same after trying a request n times as it would be after trying just once.

This comment has been minimized.

Show comment Hide comment
@robink

robink Apr 23, 2013

Ok, I get it now. Thanks a lot for the answers.

@robink

robink Apr 23, 2013

Ok, I get it now. Thanks a lot for the answers.

if retries_left > 0 then retries_left -= 1; retry else raise end
end
if response.body and !response.body.empty?

2 comments on commit cc81aa5

@pekeler

This comment has been minimized.

Show comment Hide comment
@pekeler

pekeler Mar 29, 2014

My production app experienced a bunch of sporadic timeouts using the Twilio API on Monday and Tuesday, all for sending messages which are POSTs, i.e. they were not retried. This sucked because I had to resend them manually.

I understand your reasons but what are the chances that it's just the request's response that timed out, i.e. Twilio received and processed the request? In my case, none of the requests made it through. So I'm wondering if your change is theoretically correct while not useful in practice.

How else should I deal with this problem? If the message sending failed, get all recent messages to the same number to see if the request made it through, and only if not, retry the sending? This seems complicated and error prone.

Wouldn't it make more sense, if the client simply retries, and the API server ignored duplicate requests (again, this is assuming that it can actually happen in practice that the request goes through but the response times out).

Btw, Twilio's support suggested I fork and undo this line.

Thanks,
Christian

My production app experienced a bunch of sporadic timeouts using the Twilio API on Monday and Tuesday, all for sending messages which are POSTs, i.e. they were not retried. This sucked because I had to resend them manually.

I understand your reasons but what are the chances that it's just the request's response that timed out, i.e. Twilio received and processed the request? In my case, none of the requests made it through. So I'm wondering if your change is theoretically correct while not useful in practice.

How else should I deal with this problem? If the message sending failed, get all recent messages to the same number to see if the request made it through, and only if not, retry the sending? This seems complicated and error prone.

Wouldn't it make more sense, if the client simply retries, and the API server ignored duplicate requests (again, this is assuming that it can actually happen in practice that the request goes through but the response times out).

Btw, Twilio's support suggested I fork and undo this line.

Thanks,
Christian

@carlosdp

This comment has been minimized.

Show comment Hide comment
@carlosdp

carlosdp Mar 29, 2014

Contributor

It would probably be better to catch the exception and implement your own retry logic. Otherwise, you could result in the exact duplication that the commit is supposed to prevent. It should be simple enough to just retry on exception in your application rather than fork the gem and implement it at the lower level if that's the functionality you need.

Contributor

carlosdp replied Mar 29, 2014

It would probably be better to catch the exception and implement your own retry logic. Otherwise, you could result in the exact duplication that the commit is supposed to prevent. It should be simple enough to just retry on exception in your application rather than fork the gem and implement it at the lower level if that's the functionality you need.

Please sign in to comment.