twilio / twilio-csharp

Twilio C#/.NET Helper Library for .NET6+.
MIT License
674 stars 302 forks source link

SendSmsMessage() returns null on success #99

Closed jinmatt closed 10 years ago

jinmatt commented 10 years ago
var twilio = new TwilioRestClient(accountSID, authToken);
var message = twilio.SendSmsMessage(from, to, sms_message);

if (message.RestException != null) {
        var error = message.RestException.Message;
        // handle the error ...
    }

This is how usually I check for error as per the documentation, but the message variable itself will be null if successfully SMS is send. So return something sensible if success.

devinrader commented 10 years ago

The only time that a call to SendSmsMessage should return a null is if a transport exception (like a connection timeout) is happening when RestSharp makes the request to the Twilio API,.

If you are seeing a null returned when calling SendSmsMessage, first make sure you are running the latest version of the library, then if the null value persists either submit a pull request with a fix, or send repro details and I will try to replicate the issue here. Make sure you include what platform you are using (.NET, Silverlight, Windows Phone, etc) and Visual Studio version.

JasonGloverNZ commented 10 years ago

Over the last 10 days we've had this happen 14 times, and it appears to be escalating.

We updated the Twilio library maybe 4 weeks. We are running

JasonGloverNZ commented 10 years ago

Getting around the issue by trapping the null and alerting our tech support dept.

if (messageResponse == null)
{
    NotificationServiceFactory.NotificationService.NotifyTechSupport("Error sending SMS to " + mobileNumberTidied, "twilio.SendSmsMessage returned null.\n\n" + message);
    return CreateCustomerResultCode.SmsCommunicationError;
}
kevinburke commented 10 years ago

Wondering if this has to do with the timeout that was incorrectly set to 3 seconds instead of 30 seconds - if Twilio is sending back the response headers, but not sending back the response body (or sending back neither), in the timeout period.


Kevin Burke | 415-723-4116 | www.twilio.com

On Sun, Nov 17, 2013 at 2:12 PM, Jason Glover notifications@github.comwrote:

Getting around the issue by trapping the null and alerting our tech support dept.

if (messageResponse == null){ NotificationServiceFactory.NotificationService.NotifyTechSupport("Error sending SMS to " + mobileNumberTidied, "twilio.SendSmsMessage returned null.\n\n" + message); return CreateCustomerResultCode.SmsCommunicationError;}

— Reply to this email directly or view it on GitHubhttps://github.com/twilio/twilio-csharp/issues/99#issuecomment-28665639 .

devinrader commented 10 years ago

Likely. I believe that got fixed here, so I just need to push a nuget package. I'll get that done today.

JasonGloverNZ commented 10 years ago

Has this been fixed? Because I'm looking for an update in VS NuGet and not finding one.

levka9 commented 10 years ago

Hello,

I've the same issue. The update in nuget not exist.

devinrader commented 10 years ago

New package has been uploaded.

mausch commented 10 years ago

Thanks for the fix, but IMHO returning null is not a good way to handle a transport error. What do you think about throwing a specific exception or including a timeout message in the RestException property?

devinrader commented 10 years ago

I agree its not great. RestSharp actually has a mechanism to notify you of transport errors and I've been meaning to expose that through the Twilio library.

I'm not sure if that means rethrowing that exception or just exposing it as a property. Thoughts?

mausch commented 10 years ago

I'm not familiar with RestSharp and I've only used twilio-csharp for a few days so take this with a grain of salt, but since it already returns errors through RestException instead of throwing, I think throwing in this case would be unexpected, so returning it through a property would be better.

Still, even throwing would be better than returning null.