fix: RequestValidator thread-safety

[benmccallum]

Fixes #466

Fixes

Makes RequestValidator thread-safe by not holding on to instances of the crypto algos used and instead new'ing them up and disposing of them per validation.

Checklist

• I acknowledge that all my contributions will be made under the project's license
• I have made a material change to the repo (functionality, testing, spelling, grammar)
• I have read the Contribution Guidelines and my PR follows them
• I have titled the PR appropriately
• I have updated my branch with the main branch
• I have added tests that prove my fix is effective or that my feature works
• I have added the necessary documentation about the functionality in the appropriate .md file
• I have added inline documentation to the code I modified

If you have questions, please file a support ticket, or create a GitHub Issue in this repository.

[benmccallum]

Disclaimer: I don't have any of the older target frameworks installed and don't really want to go and install them. I'm hoping CI will tell me if the test passes, but I wasn't even able to target net6.0 and run it, as the build kept complaining about multiple entry points.

If someone with all the right frameworks installed can validate the test reproduced the issue, that'd be good.

[Swimburger]

I'm trying to make the validator more efficient and stumbled on the related issue and your PR.
Your changes also make the code more efficient, so I'm considering pulling your PR into mine if you don't mind.
We can also try to get this PR into the code base first, and then I can update from main, but timing wise I don't know how fast someone can review and approve this.

PS: To avoid confusion, I work for Twilio but not on this library, so I am contributing this code from my personal free time too.
I'll try to nudge some folks to get this and my PR moving.

[Swimburger]

Quick question, would the validator still be thread safe if it implements IDisposable and is used as a singleton?
I'm wondering what the most efficient way would be.

[benmccallum]

Hey @Swimburger , thanks for pushing this along. Happy for you to merge this into your own PR (or re-make the commits on yours if that's easier, as long as you can add me via a co-author commit footer).

Re: singleton + disposable, if the validator itself instantiates instances of the algos and holds on to them, it wouldn't be thread-safe as multiple threads could use the same instance at once and hit the issue we all hit.

With my current PR here, singleton would be fine, and no need for disposable as the algo instances are new'd up and disposed of every time validation is attempted.

The alternative was to lock around the non-thread-safe call to ComputeHash, but that'd block multiple threads, so wouldn't make sense at scale IMO.

The most performant solution would be a combination of my current solution here + some #if NET6_OR_HIGHER blocks wrapping usage of use the newer one-shot methods available in .NET 6 and onwards, then falling back to what currently happens here (instantiate a new algo instance each time) for older frameworks.

Hope that helps.

[Swimburger]

Thank you for the clarification!

[benmccallum]

Superseded by #660