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

ExponentialBackoff strategy recreates Random instance each time #944

Closed
4 tasks done
watfordsuzy opened this issue Apr 10, 2024 · 4 comments · Fixed by #945
Closed
4 tasks done

ExponentialBackoff strategy recreates Random instance each time #944

watfordsuzy opened this issue Apr 10, 2024 · 4 comments · Fixed by #945
Assignees
Labels

Comments

@watfordsuzy
Copy link
Contributor

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

  1. 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 added a commit to watfordsuzy/box-windows-sdk-v2 that referenced this issue Apr 10, 2024
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 box#944
@watfordsuzy
Copy link
Contributor Author

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
Copy link
Contributor

mwwoda commented Apr 11, 2024

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 added a commit to watfordsuzy/box-windows-sdk-v2 that referenced this issue Apr 11, 2024
watfordsuzy added a commit to watfordsuzy/box-windows-sdk-v2 that referenced this issue Apr 11, 2024
@watfordsuzy
Copy link
Contributor Author

@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
Copy link
Contributor

mwwoda commented Apr 15, 2024

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

watfordsuzy added a commit to watfordsuzy/box-windows-sdk-v2 that referenced this issue Apr 15, 2024
mwwoda pushed a commit that referenced this issue Apr 15, 2024
* fix: do not recreate Random each time

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

* Use a thread safe Random instance #944

* Forgot to commit csproj change #944

* Address PR comment #944
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants