Skip to content

feat: Add std support for no-std without global rand seed#169

Merged
Xuanwo merged 14 commits intoXuanwo:mainfrom
wackazong:no_std-support
Dec 17, 2024
Merged

feat: Add std support for no-std without global rand seed#169
Xuanwo merged 14 commits intoXuanwo:mainfrom
wackazong:no_std-support

Conversation

@wackazong
Copy link
Copy Markdown
Contributor

Split #168 into two PRs as requested, this is the one for no_std support.

@wackazong wackazong mentioned this pull request Dec 1, 2024
pub use exponential::ExponentialBackoff;
pub use exponential::ExponentialBuilder;

trait Random {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, I don't want to introduce a new trait for this. Maybe we can store a new fastrand::Rng in every FibonacciBackoff?

@wackazong
Copy link
Copy Markdown
Contributor Author

Because of jitter, a random source is needed in all three Backoff Objects. That is why I used the trait. It is not just the FibonacciBackoff that needs randomness.

Considering this, I think it makes sense to use a trait in order to avoid duplication. But please, tell me how you want to proceed.

@wackazong
Copy link
Copy Markdown
Contributor Author

wackazong commented Dec 15, 2024

Reduced use of alloc to tests, this makes backon even better suitable for no_std

@Xuanwo
Copy link
Copy Markdown
Owner

Xuanwo commented Dec 16, 2024

Because of jitter, a random source is needed in all three Backoff Objects. That is why I used the trait. It is not just the FibonacciBackoff that needs randomness.

Thank you for the explanation.

But please, tell me how you want to proceed.

I think we can store a fastrand::Rng in FibonacciBackoff, which will be built when calling BackoffBuilder::build. If users don't provide one, we can rely on the global instance or use our own default value. For the default value, plese use 0x6261636b6f6e which is the hex output of backon in bytes.

With this change, we no longer need to differentiate between std and no-std.

@wackazong
Copy link
Copy Markdown
Contributor Author

I removed the trait and integrated the Rng as you suggested. Does this meet your expectations?

Copy link
Copy Markdown
Owner

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @wackazong for this! It mostly aligns with what I expected, with only a few minor suggestions.

Copy link
Copy Markdown
Owner

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @wackazong a lot for working on this. The only thing left is to address the merge conflicts.

@Xuanwo
Copy link
Copy Markdown
Owner

Xuanwo commented Dec 17, 2024

Hi @wackazong, the conflict persists even after 7668ec8 (#169). Please ensure your base is set to Xuanwo/main.

@wackazong
Copy link
Copy Markdown
Contributor Author

Ok, got it now.

Copy link
Copy Markdown
Owner

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @wackazong, let's move!

@Xuanwo Xuanwo changed the title No std support feat: Add std support for no-std without global rand seed Dec 17, 2024
@Xuanwo Xuanwo merged commit f7e3b97 into Xuanwo:main Dec 17, 2024
wackazong added a commit to wackazong/backon that referenced this pull request Dec 17, 2024
Split Xuanwo#168 into two PRs as
requested, this is the one for no_std support.

---------

Co-authored-by: Xuanwo <github@xuanwo.io>
wackazong added a commit to wackazong/backon that referenced this pull request Dec 17, 2024
Split Xuanwo#168 into two PRs as
requested, this is the one for no_std support.

---------

Co-authored-by: Xuanwo <github@xuanwo.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants