alphagov / notifications-net-client

.NET client for the GOV.UK Notify API
https://www.nuget.org/packages/GovukNotify/
MIT License
25 stars 20 forks source link

Fix synchronous API #101

Closed david-preston-triad closed 4 years ago

david-preston-triad commented 4 years ago

What problem does the pull request solve?

Earlier this year, I submitted Pull Request https://github.com/alphagov/notifications-net-client/pull/83 to add an asynchronous version of the NotificationClient API. Subsequently, an issue was found with the synchronous version of the API whereby the affected code appears to "hang" when the API is called. The issue occurs in specific scenarios, and most commonly affects the following project types:

The issue is less likely to affect .NET (Framework) Console applications or .NET Core projects. It is caused because the methods of the synchronous API are now implemented by calling the corresponding asynchronous API methods and waiting for the result. This can lead to a deadlock on the main thread used by Windows Forms and ASP.NET applications. The issue and work-around are described at https://medium.com/bynder-tech/c-why-you-should-use-configureawait-false-in-your-library-code-d7837dce3d7f.

I have tested the fix successfully with the following types of project:

Checklist

ghost commented 4 years ago

Can one of the admins verify this patch?

quis commented 4 years ago

@govuk-notify-jenkins test this please

quis commented 4 years ago

@david-preston-triad thanks for looking into this. We’ll find an appropriate person to review it.

leohemsted commented 4 years ago

@david-preston-triad hi, thanks for the PR! Really sorry for how long it's taken for us to get round to this, but we think this looks good (though we're not domain experts here). We'll get this deployed this evening or tomorrow morning (but from another PR just so I could rebase it - https://github.com/alphagov/notifications-net-client/pull/105 )