Vonage / vonage-dotnet-sdk

Vonage REST API client for .NET, written in C#. API support for SMS, Voice, Text-to-Speech, Numbers, Verify (2FA) and more.
https://developer.vonage.com/
Apache License 2.0
106 stars 86 forks source link

VerifyClient.VerifyRequestAsync hangs when called in an asp.net 4.7 application #574

Closed tim-arheit closed 6 months ago

tim-arheit commented 6 months ago

When calling var result = await client.VerifyClient.VerifyRequestAsync(req, _credentials).ConfigureAwait(false); The task simply never returns. It does successfully call the verify API as I do get a text message.

After downloading the vonage-dotnet-sdk to troubleshoot it, I found numerous awaits in VerifyClient.cs and ApiRequest did not have a .ConfigureAwait(false); (which can in certain cases cause a task to wait indefinitely) After fixing all those cases and VerifyRequestAsync, and VerifyCheckAsync both returned as expected.

I didn't test the other API methods, but I suspect many of them also have the same issue.

Tr00d commented 6 months ago

Hi @tim-arheit,

Thanks for investigating. It's a weird scenario though - ConfigureAwait(false) should not provide a different behavior as it's mainly about dealing with the synchronization context.

I'll have a look at it and get back to you.

tim-arheit commented 6 months ago

I agree, it's exceedingly rare and difficult to cause in a small test project. I've only run into this behavior (in other code) once before. Without ConfigureAwait(false) await resumes on the original thread, but it seems in this case, in this project, something took over that thread causing the await to wait indefinitely. Simply adding .ConfigureAwait(false) fixed the issue for me and offhand, I didn't see any issues with losing synchronization context.

Note I've used the vonage SDK in other projects with no issues before. The particular project I'm using it in this time is an asp.net application which seems to start various threads as part of the framework itself. It's an old project that really doesn't use async await otherwise.

tim-arheit commented 6 months ago

One other thing. All the previous projects I used the SDK with were .core. According to this article (https://corebts.com/blog/async-await-configureawait/), Microsoft did away with the SynchronizationContext that caused the need to use ConfigureAwait(false) (even though it's still recommended to use ConfigureAwait(false)), but if .Net Framework calls the library synchronously (and ConfigureAwait(false) is missing), there will be issues. I suspect this is the issue I am having.

Tr00d commented 6 months ago

Yes, indeed - you're absolutely correct.

I investigated that topic earlier today and I learned that it's recommended to use ConfigureAwait when working on a library, given you don't know if the caller is going to need the synchronization context or not. So, it's better to leave that open, knowing there are rare cases where you can have a deadlock... For what I've seen, it's mostly with ASP.NET.

I'll tackle that problem. Hopefully, some automated refactoring can take care of that for me.

Tr00d commented 6 months ago

@tim-arheit The version 7.2.1 has been released and should solve this problem.

Can you give it a try and let me know if everything's okay on your side?

tim-arheit commented 6 months ago

7.2.1 does fix the issue we were having. Thanks!

Tr00d commented 6 months ago

Great to hear! :)