Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Asynchronous Signing in IJwtAlgorithm interface #468

Open
AliKhalili opened this issue Mar 14, 2023 · 5 comments
Open

Support Asynchronous Signing in IJwtAlgorithm interface #468

AliKhalili opened this issue Mar 14, 2023 · 5 comments
Assignees

Comments

@AliKhalili
Copy link

Currently, the IJwtAlgorithm interface does not support asynchronous signing, but the AWS KMS Client has implemented the Sign method in an async manner. As a result, when implementing a custom signing algorithm that uses the KMS Client as the underlying method for actual signing, the current solution involves blocking the calling thread using the GetResult() method. This approach may lead to resource starvation and deadlock if the workload is high.

To enable non-blocking signing using AWS KMS Client, we need to add support for asynchronous signing in the IJwtAlgorithm interface.

any thoughts or comments, please?

@abatishchev
Copy link
Member

abatishchev commented Mar 14, 2023

Hi @AliKhalili, thanks for reaching out and proposing an improve to the library!

Overall I don't mind and welcome it. Just needs some discussion first. I see two options:

  1. Change the interface to make all methods async

Pros:

  • only 1 method to call, no confusion

Cons:

  • 99,9..9% of scenarios are sync, this is the first every request to make it async. What adds a small but overhead, probably can be eased by leveraging a value task
  • a breaking change, constitutes bumping major version to 11.0
  1. Add a method to the interface

Pros:

  • tailored method signature to this scenario
  • isn't a breaking change, constitutes only minor version bump to 11.1

Cons:

  • introduces confusion (which method to call and when) and complexity (what would be the default implementation's behavior - wrap the sync result with a (value) task or throw an exception?)

What would be your take?

@AliKhalili
Copy link
Author

Hi @abatishchev,

Thank you for your response. I appreciate your openness to discussing this matter further.

While it is true that the majority of scenarios may be synchronous, it is becoming increasingly common for developers to expect asynchronous interfaces, especially in scenarios where cloud services(Cloud key management, Cloud HSM) are involved.

On the other hand, as you mentioned, the majority of the consumers are not expected to use the asynchronous methods, so it does not make sense to update the interface to make all methods asynchronous(in some cases it would be nearly impossible to change a legacy code base to be async and works without any problem).

Therefore, I would recommend providing both synchronous and asynchronous versions of each method. This allows consumers to choose which version they want to use based on their needs.
However, this approach can lead to code duplication and may make the interface more difficult to maintain.

@abatishchev
Copy link
Member

Hi @AliKhalili,
I was sick past 2 months so couldn't come back to this.
You haven't followed up either. Is this something you're still interested in? If yes, can you contribute at least a draft of the proposed change?

@AliKhalili
Copy link
Author

AliKhalili commented Apr 27, 2023

Certainly, I hope you are feeling well.
Yes, I am interested in your request. Thank you for reaching out to me. Please allow me some time to work on it and I will be happy to provide you with a draft PR as soon as possible.

@drusellers
Copy link
Contributor

Hello Everyone. Would just like to add that I would love to see IAlgorithmFactory.Create be made async as well. This may not be the best approach, but I've currently implemented a custom IAlgorithmFactory that looks up certificates in AWS SSM. Since this is a web request having support for async would be nice. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants