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

Fix sequence generation to be always increasing #6

Merged
merged 9 commits into from
Feb 24, 2024

Conversation

Qqwy
Copy link
Contributor

@Qqwy Qqwy commented Feb 23, 2024

  • Fixes generation of the normal generator
  • Fixes generation of the sync generator
  • Replaces the two incorrect test_generate tests with test_generate_ordered tests
  • Gets rid of MaybeUninit inside the sync generation. I've looked at the created assembly (using cargo asm) and the compiler is more than clever enough to optimize the wrapping and unwrapping into an Option away here, so there's no need for unsafe. (Extra fun as it was the only unsafe in this crate, so now it's fully no-unsafe!)

Fixes #5

Copy link
Owner

@MrGunflame MrGunflame left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, please run cargo fmt on the thing once. Looks almost fine besides that.

src/lib.rs Outdated Show resolved Hide resolved
src/sync.rs Outdated Show resolved Hide resolved
@Qqwy
Copy link
Contributor Author

Qqwy commented Feb 24, 2024

Thanks for reviewing 😊 ! Your feedback has been applied.

I'm running the loom tests locally now (I did not realize that they didn't run as part of the normal test suite before), using RUSTFLAGS="--cfg loom" cargo test --lib --all-features --release, and see that the no_duplicates_no_wrap test is failing.
I'll investigate what is going on there.

@Qqwy
Copy link
Contributor Author

Qqwy commented Feb 24, 2024

The trick was that since the sequence stores the 'next' value, at the very start we have to initialize it at 1. Otherwise, in the edge case where someone tries to generate a snowflake in the very same millisecond as the epoch, it would immediately trigger the wait condition.

In real programs this won't ever come up of course, since time will always have passed since the epoch.

@Qqwy Qqwy requested a review from MrGunflame February 24, 2024 11:38
@Qqwy Qqwy force-pushed the 5-fix_sequence_generation branch from a988bf4 to 9502781 Compare February 24, 2024 16:21
@MrGunflame
Copy link
Owner

This is very strange, I have no idea why cargo fmt would break formatting and then not fix it.

@Qqwy
Copy link
Contributor Author

Qqwy commented Feb 24, 2024

Hang on; I think I might be running a slightly older version of cargo fmt locally (from 2023-11) (My nightly version was up-to-date but my stable version wasn't). Let me see whether updating to the latest version fixes it.

@Qqwy Qqwy force-pushed the 5-fix_sequence_generation branch from 9502781 to f61231b Compare February 24, 2024 16:27
@Qqwy
Copy link
Contributor Author

Qqwy commented Feb 24, 2024

That should've done it 👍

Copy link
Owner

@MrGunflame MrGunflame left a comment

Choose a reason for hiding this comment

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

Just a few leftovers from debugging it seems.

src/sync.rs Outdated Show resolved Hide resolved
src/sync.rs Outdated Show resolved Hide resolved
Qqwy and others added 2 commits February 24, 2024 17:35
Co-authored-by: MrGunflame <[email protected]>
Co-authored-by: MrGunflame <[email protected]>
@Qqwy
Copy link
Contributor Author

Qqwy commented Feb 24, 2024

Just a few leftovers from debugging it seems.

Apologies, that was sloppy. I should stop trying to make these kinds of PRs late at night. Thanks for your keen eye!

Copy link
Owner

@MrGunflame MrGunflame left a comment

Choose a reason for hiding this comment

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

No need to apologize, thanks.

@MrGunflame MrGunflame merged commit 566bedb into MrGunflame:master Feb 24, 2024
5 checks passed
@MrGunflame
Copy link
Owner

Released in version 1.0.3.

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.

Sequence generation is not correct: it should reset when the timestamp increases
2 participants