search

aws / aws-sdk-net-extensions-cognito

An extension library to assist in the Amazon Cognito User Pools authentication process
Apache License 2.0
102 stars 49 forks source link

#129 - Fix StartWithSrpAuthAsync called by Sync context #130

Closed swensorm closed 10 months ago

swensorm commented 10 months ago

Issue #, if available: https://github.com/aws/aws-sdk-net-extensions-cognito/issues/129

Description of changes: Fixed bug with StartWithSrpAuthAsync(InitiateSrpAuthRequest) that would result in a deadlock scenario if the method was called by a synchronous context (e.g. .Result called on the method).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

philasmar commented 10 months ago

I am confused as to why there are 2 PRs with differing commits. https://github.com/aws/aws-sdk-net-extensions-cognito/pull/131 and https://github.com/aws/aws-sdk-net-extensions-cognito/pull/130. Could you please close one and consolidate your changes into a single PR?

swensorm commented 10 months ago

@normj I have made the changes everywhere I could find to both Pull Requests in their respective approaches (this PR to add .ConfigureAwait(false), the other to remove it where not necessary). I changed the unit tests for consistency, though this was not required.

There is 1 missing from the README.md (line 181) but I am conflicted if we want to add that as it is a bit presumptuous to assume consumers need it as it's only required if they consume it from a synchronous context. It is in the GetS3BucketsAsync method above there but that one is already a bit invalid as it's a void method which is also not recommended practice with async programming. Please let me know if you would like me to add it on 181 though.

Thank you for your review!

swensorm commented 10 months ago

Hi @philasmar. As discussed in https://github.com/aws/aws-sdk-net-extensions-cognito/issues/129 there are 2 approaches to resolving the deadlocks. One is to add .ConfigureAwait(continueOnCapturedContext: false) to all awaits currently missing it. The other is to remove the unnecessary async/await wrapping and state machine generation from the methods which do not require it.

I am leaving it up to the AWS team to decide which approach they would prefer to go with and close the other.

philasmar commented 10 months ago

Hi @philasmar. As discussed in #129 there are 2 approaches to resolving the deadlocks. One is to add .ConfigureAwait(continueOnCapturedContext: false) to all awaits currently missing it. The other is to remove the unnecessary async/await wrapping and state machine generation from the methods which do not require it.

I am leaving it up to the AWS team to decide which approach they would prefer to go with and close the other.

Thanks for clarifying. I have closed the other PR in favor of this one. I'll continue reviewing this one.

swensorm commented 10 months ago

Thank you all for reviewing! Happy to be a part of the project!

normj commented 10 months ago

The PR has been released as part of version 2.5.1. Thanks again for the PR.

swensorm commented 10 months ago

Wonderful news, thank you very much! I will pull this into our project tomorrow.