fix: do not recreate Random each time

[watfordsuzy]

On .NET Framework it is incorrect to recreate Random instances each time, these should instead be one per instance. On .NET Core and later this is not a limitation, but the libraries share a codebase.

Fixes #944

[mwwoda]

Hi @watfordsuzy
Thanks for the contribution! I think it's a good practice in general reuse the Random instance so we could do it for both of the SDKs (Framework and Core). I think thread safety could be a potential issue there (e.g. in chunked file uploads) https://learn.microsoft.com/en-us/dotnet/fundamentals/runtime-libraries/system-random#thread-safety. What do you think?

[watfordsuzy]

For .NET Core and beyond you don't strictly need to re-use it (but should) as the underlying implementation already gives you a Thread Safe implementation:
https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Random.cs

On .NET Framework a thread local Random could be used instead, but there are some interesting caveats if you go and read the comments regarding ThreadSafeRandom used in .NET Core+.

[mwwoda]

There is also an explicit thread safe implementation for Random https://learn.microsoft.com/en-us/dotnet/api/system.random.shared?view=net-8.0, but only for .NET 6+ and we need to support older versions too.

If you want to work on this further, I think a thread-safe solution is the way to go. The overhead of potential locks shouldn't be a big deal, since this feature is already used to delay subsequent api calls, so that would just mean a slight bigger delay. While arguably making it not thread-safe isn't such a big deal either, it might result in slightly different behavior in multi-threaded environment compared to the single-threaded one. If necessary it's also possible to provide two implementation for both SDKs with pragmas. See example

box-windows-sdk-v2/Box.V2/Request/HttpRequestHandler.cs

Line 286 in b410dfd

#if NETSTANDARD2_0

[watfordsuzy]

I've gone ahead and made the suggested change, adapting their internal ThreadSafeRandom to fit the existing code. I've also added a section for .NET 6+, delegating to the inboxed thread safe random implementation.

[watfordsuzy]

I don't believe the Integration Test failures are related @mwwoda

[mwwoda]

Don't worry about the integration tests. Unfortunately, we cannot currently run them on forks.

[mwwoda]

The code looks pretty good. I see that you have introduced a modern implementation for .NET6+ too which is great. Unfortunately, I just checked that when you use a conditional compilation flag such as NET6_0_OR_GREATER when not explicitly targeting this version in the library, this code will be never used during compilation (so it is basically dead code at this point). We would have to explicitly target .NET 6 version, which I don't think we will tackle in the near future. Maybe you could remove implementation for .NET 6+, and then we can merge your change. Sorry for the confusion.

[watfordsuzy]

@mwwoda no worries!

[mwwoda]

LGTM! Will be included in the next release of the SDK.