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

Async methods should ALWAYS accept an optional cancellation token #113

Closed GabrielHare closed 1 year ago

GabrielHare commented 1 year ago

Describe the feature

Feature: All async methods should accept an optional cancellation token.

Examples of methods that do not satisfy these requirements:

Pertains to package version 2.4.0 (latest version on NuGet as of this time)

Use Case

Observation: authorization requests NEVER timeout - even if there is no network connection. I have tried waiting for an hour, and the authorization will just wait until enable a network connection.

Use-case: If an application is running with NO network connection, a thread that awaits an authorization request will never restart. However, if the authorization is a background task it cannot be cancelled, so an IDisposable pattern can't work and a resource leak is likely. Furthermore, the standard SDK methods all accept cancellation tokens, so this pattern change is really odd.

Work-around: Use the SDK source code and make the necessary changes.

Proposed Solution

Have an optional CancellationToken argument in the StartWithSrpAuthAsync method. Pass this to the invoked SDK methods (which already have optional cancellation tokens).

That's all it would take... just stick with the existing design pattern. Since the arguments are optional no code that currently uses these methods would need to change.

Other Information

No response

Acknowledgements

AWS .NET SDK and/or Package version used

AWS SDK Extensions for .NET Standard 2.0 (compatible with .NET Framework 4.5) Version 2.4.0 https://www.nuget.org/packages/Amazon.Extensions.CognitoAuthentication/

Targeted .NET Platform

.NET Framework 4.5

Operating System and version

Ubuntu 22.04

ashishdhingra commented 1 year ago

Appears to be a valid feature request following the standard async method signature. Needs review with the team.

@GabrielHare Thanks for submitting the feature request. Feel free to contribute a PR that could be reviewed by the team.

GabrielHare commented 1 year ago

Looking at the PR I noticed that some CR / NL changes occurred. I think it's in the direction that is consistent with the project...

ashishdhingra commented 1 year ago

@GabrielHare Thanks for the contribution. The change has been released in Amazon.Extensions.CognitoAuthentication version 2.4.1

github-actions[bot] commented 1 year ago

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see. If you need more assistance, please either tag a team member or open a new issue that references this one. If you wish to keep having a conversation with other community members under this issue feel free to do so.