feat: Optimize request validator + make thread safe (backwards compatible)

[Swimburger]

All third-party contributors acknowledge that any contributions they provide will be made under the same open-source license that the open-source project is provided under.

ASP.NET Core is pushing performance with every release, and I received feedback that the RequestValidator is inefficient. This is not my area of expertise, so there's probably more optimization possible, especially if we can drop older versions of .NET, but I gave it a stab while trying to keep it backwards compatible.

The only breaking change would be that I'm doing some additional input validation which previously would have thrown null reference exceptions instead of argument exceptions. Technically a breaking change, but in reality I don't expect anyone to run into this issue.

Fixes

I created a benchmark project with a copy of the original validation code while improving the existing code. The benchmark tests for happy path, where the parameters are passed in as a dictionary and the signature matches for the original URL, not needing to perform the validation for the second URL variant. The unhappy path passes in a name value collection, and doesn't pass on the URL that's passed in, but does match on the variant URL.

The code is more performant because some of the string manipulations that used to be done for every URL is only done once now, and if the first signature matches, the validator doesn't bother checking for the variant URL. Other than that, there are minor enhancements trying to reduce the amount of objects needed to perform the validation.

Here's the benchmark results for .NET 6:

BenchmarkDotNet=v0.13.3, OS=Windows 11 (10.0.22621.963)
Intel Core i7-8700 CPU 3.20GHz (Coffee Lake), 1 CPU, 12 logical and 6 physical cores
.NET SDK=7.0.101
  [Host]     : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2
  DefaultJob : .NET 7.0.1 (7.0.122.56804), X64 RyuJIT AVX2

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
OriginalUnhappyPath	15.590 us	0.1598 us	0.1416 us	1.9836	0.2441	12.27 KB
CurrentUnhappyPath	12.526 us	0.2482 us	0.2759 us	1.3885	-	8.52 KB
OriginalHappyPath	8.906 us	0.1709 us	0.2161 us	1.6327	0.2289	10.02 KB
CurrentHappyPath	2.721 us	0.0240 us	0.0224 us	0.6256	-	3.85 KB

All the tests continue to pass.

This PR also contains the commits from PR #598 to fix the thread safety issue (fixing #466), and as a result it also improves performance. Thank you @benmccallum.

Checklist

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

[Swimburger]

I'm leaving this PR as a draft for now, as you may want me to remove the benchmark project before pulling it in. If you have any suggestions on how to improve the performance, let me know!

[Swimburger]

Here's an allocation report from VS when running the current unhappy path: Report20221229-2230.zip

And the relevant call tree:

Function Name	Total (Allocations)	Self (Allocations)	Self Size (Bytes)	Module Name
+ RequestValidationBenchmark.CurrentUnhappyPath()	270	39	5,666	twilio.benchmark
+ Twilio.Security.RequestValidator.Validate(string, System.Collections.Generic.IDictionary<string, string>, string)	99	0	0	twilio.benchmark
+ Twilio.Security.RequestValidator.GetUriVariation(string)	48	3	184	twilio.benchmark
- [External Call] System.Uri.ctor(string)	37	37	2,684	system.private.uri.il
- [External Call] System.UriBuilder.SetFieldsFromUri()	7	7	384	system.private.uri.il
+ [Allocations]		3	184
-  System.UriBuilder		1	88
-  System.Uri		1	56
-  System.String		1	40
- [External Call] System.Number.UInt32ToDecStr(uint)	1	1	30	System.Private.CoreLib.il
+ Twilio.Security.RequestValidator.GetJoinedParametersStringBuilder(System.Collections.Generic.IDictionary<string, string>)	21	2	104	twilio.benchmark
- [External Call] System.Text.StringBuilder.AppendWithExpansion(ref char, int)	10	10	1,832	System.Private.CoreLib.il
- [External Call] System.Array.Sort<T>(!!0[], int, int, System.Collections.Generic.IComparer<T>)	7	7	272	System.Private.CoreLib.il
- [External Call] System.Collections.Generic.EnumerableHelpers.ToArray<T>(System.Collections.Generic.IEnumerable<T>)	1	1	184	system.linq.il
+ [Allocations]		2	104
-  System.Char[16]		1	56
-  System.Text.StringBuilder		1	48
- [External Call] System.Collections.Generic.Dictionary<T, T>.get_Keys()	1	1	24	System.Private.CoreLib.il
- [External Call] System.Text.StringBuilder.ToString()	3	3	1,876	System.Private.CoreLib.il
+ Twilio.Security.RequestValidator.GetValidationSignature(string)	8	0	0	twilio.benchmark
- [External Call] System.Text.Encoding.GetBytes(string)	2	2	911	System.Private.CoreLib.il
- [External Call] System.Security.Cryptography.HashAlgorithm.CaptureHashCodeAndReinitialize()	5	5	218	system.security.cryptography.il
- [External Call] System.Security.Cryptography.HashAlgorithm.ComputeHash(uint8[])	1	1	42	system.security.cryptography.il
+ Twilio.Security.RequestValidator.SetPort(string, System.UriBuilder, int)	13	5	272	twilio.benchmark
+ [Allocations]		5	272
-  System.String		4	224
-  System.Text.StringBuilder		1	48
- [External Call] System.UriBuilder.ToString()	5	5	222	system.private.uri.il
- [External Call] System.Text.StringBuilder.ctor(string, int, int, int)	1	1	108	System.Private.CoreLib.il
- [External Call] System.String.Substring(int, int)	2	2	78	System.Private.CoreLib.il
- [External Call] System.Text.StringBuilder.Insert(int, string)	4	4	322	System.Private.CoreLib.il
- [External Call] System.Convert.ToBase64String(uint8[])	2	2	156	System.Private.CoreLib.il
- RequestValidationBenchmark.cctor()	83	2	160	twilio.benchmark
- [Allocations]		39	5,666
+ Twilio.Security.RequestValidator.ToDictionary(System.Collections.Specialized.NameValueCollection)	10	1	80	twilio.benchmark
- [External Call] System.Collections.Generic.Dictionary<T, T>.TryInsert(T, T, System.Collections.Generic.InsertionBehavior)	8	8	1,984	System.Private.CoreLib.il
- [External Call] System.Collections.Specialized.NameValueCollection.get_AllKeys()	1	1	184	system.collections.specialized.il
+ [Allocations]		1	80
+  System.Collections.Generic.Dictionary<System.String, System.String>		1	80
- [External Call] System.Security.Cryptography.HMACSHA1.ctor(uint8[])	34	34	1,542	system.security.cryptography.il
- [External Call] System.Security.Cryptography.SHA256+Implementation.ctor()	4	4	176	system.security.cryptography.il
- [External Call] System.Text.UTF8Encoding+UTF8EncodingSealed.GetBytesForSmallInput(string)	1	1	29	System.Private.CoreLib.il

Allocations tab:

Type	Allocations
- System.Char[]	85
- System.String	2,248
- System.Diagnostics.Tracing.EventSource.EventMetadata[]	7
- System.Byte[]	342
- System.Reflection.RuntimeParameterInfo	676
- System.Object[]	518
- System.Collections.Generic.Dictionary<,>.Entry[]	39
- System.Reflection.RuntimeMethodInfo	255
- System.Diagnostics.Tracing.EventParameterInfo[]	148
- System.Signature	237
- System.Int32[]	51
- System.Reflection.ParameterInfo[]	295
- System.RuntimeMethodInfoStub	154
- System.UInt64[]	3
- System.RuntimeType[]	195
- System.Diagnostics.Tracing.EventAttribute	149
- System.Reflection.RuntimeMethodBody	148
- System.Reflection.CustomAttributeRecord[]	156
- System.Reflection.RuntimeMethodInfo[]	29
- System.Reflection.Emit.OpCode	226
- System.Diagnostics.Tracing.EventAttribute[]	149
- System.String[]	35
- System.Text.StringBuilder	85
- System.Reflection.MdFieldInfo	57
- System.Byte	148
- System.Diagnostics.Tracing.EventLevel	148
- System.Reflection.RuntimeExceptionHandlingClause[0]	148
- System.Reflection.RuntimeLocalVariableInfo[0]	148
- System.Diagnostics.Tracing.EventKeywords	147
- System.RuntimeType.RuntimeTypeCache	23
- System.RuntimeType	81
- System.Reflection.MethodInfo[]	2
- System.Collections.Hashtable.Bucket[]	6
- System.Reflection.RuntimeFieldInfo[]	17
- System.Globalization.CultureData	4
- System.Collections.Generic.Dictionary<,>	15
- System.Reflection.Emit.DynamicILGenerator	6
- System.Int64	37
- System.Reflection.RuntimePropertyInfo	8
- System.Collections.ArrayList	21
- System.RuntimeType.RuntimeTypeCache.MemberInfoCache<>	12
- System.Reflection.Emit.DynamicMethod	6
- System.Reflection.FieldInfo[]	5
- System.Collections.Specialized.NameObjectCollectionBase.NameObjectEntry	20
- System.Diagnostics.Tracing.ScalarTypeInfo	10
- System.Func<System.Object, System.Diagnostics.Tracing.PropertyValue>	10
- System.Reflection.Emit.SignatureHelper	12
- System.UriParser.BuiltInUriParser	17
- System.Type[]	11
- System.Reflection.RuntimePropertyInfo[]	11
- System.Collections.Generic.List<>	14
- System.Globalization.CultureInfo	4
- System.Reflection.Emit.DynamicResolver	6
- System.Reflection.RuntimePropertyInfo[][]	4
- System.Diagnostics.Tracing.EventSource.OverrideEventProvider	4
- System.Reflection.MethodInvoker	8
- System.Reflection.InvokerEmitUtil.InvokeFunc	6
- System.Diagnostics.Tracing.RuntimeEventSource	1
- System.Object	13
- System.Int32	12
- System.RuntimeMethodHandle	12
- System.Reflection.Emit.DynamicMethod.RTDynamicMethod	6
- System.Reflection.Emit.ScopeTree	6
- System.Reflection.CerHashtable<,>.Table	7
- System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>, System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>.Node[31]	1
- Interop.Advapi32.EtwEnableCallback	4
- System.Diagnostics.Tracing.EventOpcode	9
- System.Diagnostics.Tracing.EventTask	9
- System.Collections.Hashtable	3
- System.IntPtr[10]	2
- System.Double[23]	1
- System.Globalization.CalendarData[23]	1
- System.Collections.Generic.HashSet<System.RuntimeType>.Entry[11]	1
- System.UInt32	8
- System.Reflection.Emit.GenericFieldInfo	6
- System.Reflection.MemberFilter	3
- System.RuntimeFieldInfoStub	3
- System.Diagnostics.Tracing.NativeRuntimeEventSource	1
- System.Diagnostics.Tracing.EventCommandEventArgs	2
- System.Globalization.CalendarData	1
- System.Diagnostics.Tracing.ManifestBuilder	1
- System.Reflection.Emit.DynamicScope	6
- System.RuntimeTypeHandle	6
- System.Reflection.RtFieldInfo	2
- Internal.Win32.SafeHandles.SafeRegistryHandle	4
- System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>.Node	2
- System.ExecutionEngineException	1
- System.OutOfMemoryException	1
- System.StackOverflowException	1
- System.Collections.Generic.Dictionary<,>.KeyCollection	5
- System.Globalization.CompareInfo	2
- System.Collections.Generic.KeyValuePair<SessionInfo, System.Boolean>[]	2
- System.Reflection.RuntimeAssembly	2
- System.Security.Cryptography.HashProviderCng	2
- System.Diagnostics.Tracing.EventProvider.SessionInfo[8]	1
- System.UriBuilder	1
- System.Diagnostics.Tracing.EventSource[]	2
- System.Collections.Specialized.NameValueCollection	1
- System.Reflection.RuntimeFieldInfo[7][]	1
- System.Reflection.RuntimeMethodInfo[7][]	1
- System.RuntimeType[7][]	1
- System.Uri.UriInfo	1
- System.Collections.Generic.GenericArraySortHelper<>	3
- System.Security.Cryptography.HMACSHA1	1
- Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle	2
- Microsoft.Win32.SafeHandles.SafeBCryptHashHandle	2
- System.Collections.Generic.HashSet<System.RuntimeType>	1
- System.Comparison<System.String>	1
- System.Diagnostics.Tracing.EventProvider.SessionInfoCallback	1
- System.Globalization.TextInfo	1
- System.Reflection.RuntimeModule	1
- System.Uri	1
- System.Uri.MoreInfo	1
- Internal.Win32.RegistryKey	2
- System.Collections.Generic.GenericEqualityComparer<>	2
- System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalComparer	2
- System.Diagnostics.Tracing.EtwEventProvider	2
- System.Diagnostics.Tracing.EventPipeEventProvider	2
- System.Diagnostics.Tracing.TraceLoggingEventHandleTable	2
- System.WeakReference<System.Diagnostics.Tracing.EventSource>	2
- System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>	1
- System.Collections.Concurrent.ConcurrentDictionary<System.ValueTuple<System.String, BCryptOpenAlgorithmProviderFlags>System.ValueTuple<Microsoft.Win32.SafeHandles.SafeBCryptAlgorithmHandle, System.Int32>>.Tables	1
- System.RuntimeType.ActivatorCache	1
- System.Security.Cryptography.HMACCommon	1
- System.Security.Cryptography.SHA256.Implementation	1
- System.Text.UTF8Encoding.UTF8EncodingSealed	1
- System.Diagnostics.Tracing.EventSourceAttribute	1
- System.WeakReference<System.Diagnostics.Tracing.EventSource>[2]	1
- System.CultureAwareComparer	1
- System.Diagnostics.Tracing.ActivityTracker	1
- System.Diagnostics.Tracing.EventSourceAttribute[1]	1
- System.Guid	1
- Twilio.Security.RequestValidator	1
- System.Globalization.CalendarId[1]	1
- RequestValidationBenchmark	1
- System.Collections.Generic.Dictionary<System.String, System.Type>.ValueCollection	1
- System.Collections.Generic.GenericComparer<System.String>	1
- System.Collections.Generic.NonRandomizedStringEqualityComparer.OrdinalIgnoreCaseComparer	1
- System.Collections.Generic.ObjectEqualityComparer<System.RuntimeType>	1
- System.DBNull	1
- System.Diagnostics.Tracing.EventPipeMetadataGenerator	1
- System.Diagnostics.Tracing.PropertyValue.<>c	1
- System.OrdinalCaseSensitiveComparer	1
- System.OrdinalIgnoreCaseComparer	1
- System.Reflection.Missing	1
- System.Text.DecoderReplacementFallback	1
- System.Text.EncoderReplacementFallback	1
- System.Type.<>c	1

We could further improve performance if we don't have to preserve the original casing of the URL, and if we didn't have to check for the URL with and without port.

[Swimburger]

Pulling the code from #598 with minor modifications improves the performance and reduces allocations further.

Method	Mean	Error	StdDev	Gen0	Gen1	Allocated
OriginalUnhappyPath	15.204 us	0.1571 us	0.1226 us	1.9836	0.2441	12.27 KB
CurrentUnhappyPath	11.855 us	0.2103 us	0.3148 us	1.4191	-	8.75 KB
OriginalHappyPath	9.241 us	0.1789 us	0.1757 us	1.6327	0.2289	10.02 KB
CurrentHappyPath	3.135 us	0.0627 us	0.0586 us	0.6676	0.0038	4.09 KB

TBD if I'm merging that PR into this one.

[Swimburger]

@davidfowl @vcsjones @benmccallum any last suggestions before I turn this into a real PR?

[benmccallum]

Much better, thanks again for pushing this along.

I've only reviewed on my phone, but it looks good to me and most importantly is thread-safe, so the docs as they were (and I imagine still are) won't lead someone down a path of great misfortune :)

[Swimburger]

C'mon Twilio folks, this code contribution has been ready to merge on the internal repo and public repo for a long time. The longer you let these things rot, the more conflicts will arise.

By not merging this, you're choosing to keep a slower and less memory-efficient version around for no reason. 🔥 🌳

[tiwarishubham635]

Hello @Swimburger! I am looking at this PR from twilio. Can you please update this PR with the latest changes to ensure that the .csproj file has no conflicts? Thanks!

[Swimburger]

Conflict resolved

[tiwarishubham635]

Review in process

[benmccallum]

We even had a review from David Fowler 😄 LGTM!