ExponentialBackoff strategy recreates Random instance each time

watfordsuzy commented 7 months ago

[X] I have checked that the SDK documentation doesn't solve my issue.
[X] I have checked that the API documentation doesn't solve my issue.
[X] I have searched the Box Developer Forums and my issue isn't already reported (or if it has been reported, I have attached a link to it, for reference).
[X] I have searched Issues in this repo and my issue isn't already reported.

Description of the Issue

Within the ExponentialBackoff retry strategy, the Random instance is recreated each time GetRetryTimeout is called. This is an anti-pattern, and incorrect for .NET Framework based projects:

In .NET Framework, the default seed value is derived from the system clock, which has finite resolution. As a result, different Random objects that are created in close succession by a call to the parameterless constructor have identical default seed values and, therefore, produce identical sets of random numbers. You can avoid this problem by using a single Random object to generate all random numbers. You can also work around it by generating your own random seed value and passing it to the Random(Int32) constructor. For more information, see the Random(Int32) constructor.

https://learn.microsoft.com/en-us/dotnet/api/system.random.-ctor

Note: this does not apply to Box.V2.Core, but they share the same repository.

Steps to Reproduce

Execute multiple ExponentialBackoff.GetRetryTimeout calls in a row on .NET Framework 4.6.2+ targets

Expected Behavior

A single Random instance is used in accordance with .NET Framework best practices.

Error Message, Including Stack Trace

N/A

Screenshots

N/A

Versions Used

.Net SDK: Box.V2 5.7.0 Windows: N/A

watfordsuzy commented 7 months ago

I also noticed that BoxConfig.RetryStrategy is only used within the Auth handler and isn't propagated to the HttpRequestHandler. This seems like an oversight.

Unfortunately adding a new parameter to the HttpRequestHandler constructor, which already has two optional parameters, would be a breaking change. If that's okay to take, I'm happy to provide that fix as well.

mwwoda commented 7 months ago

I don't think we would make a breakthrough change for such a small change at this point. But if it were possible to extend HttpRequestHandler with an optional argument and modify the SDK to use it in a non-breaking way, that would probably be fine.

watfordsuzy commented 7 months ago

@mwwoda I put together a non-breaking way to share the IRetryStrategy with HttpRequestHandler:

https://github.com/box/box-windows-sdk-v2/compare/main...watfordsuzy:box-windows-sdk-v2:issue-XXX-plumb-through-retrystrategy?expand=1

mwwoda commented 7 months ago

Nice! Maybe, you could create a PR based on this, so we can discuss it here?

box / box-windows-sdk-v2