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

feat: Add a loom implementation for event-listener #126

Merged
merged 1 commit into from
Mar 30, 2024
Merged

Conversation

jbr
Copy link
Contributor

@jbr jbr commented Mar 28, 2024

The loom tests aren't particularly useful yet, but the crate compiles and runs under loom now.

Note: This crate does not currently compile under loom without std because concurrent-queue does not compile with that combination (can't find spin_loop).

closes #23

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

I'm surprised concurrent-queue Loom tests are failing, but I guess that's due to loom not being checked in CI there.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
notgull added a commit to smol-rs/concurrent-queue that referenced this pull request Mar 30, 2024
I'm unsure why this wasn't added to begin with. This adds Loom testing
to the CI with a low number of max pre-emptions, in order to avoid
making the test take forever.

cc smol-rs/event-listener#126 (comment)

Signed-off-by: John Nunley <dev@notgull.net>
@notgull
Copy link
Member

notgull commented Mar 30, 2024

I added Loom to the CI in smol-rs/concurrent-queue#62. It looks like it works. Maybe it's the optional feature flag that's gumming it up.

notgull added a commit to smol-rs/concurrent-queue that referenced this pull request Mar 30, 2024
I'm unsure why this wasn't added to begin with. This adds Loom testing
to the CI with a low number of max pre-emptions, in order to avoid
making the test take forever.

cc smol-rs/event-listener#126 (comment)

Signed-off-by: John Nunley <dev@notgull.net>
@jbr
Copy link
Contributor Author

jbr commented Mar 30, 2024

I'm surprised concurrent-queue Loom tests are failing, but I guess that's due to loom not being checked in CI there.

To be clear, it's not that the concurrent-queue loom tests are failing, or even that concurrent-queue can't build under loom. This PR for event listener correctly builds concurrent-queue under loom and runs tests against concurrent-queue loom.

The issue is this specific scenario for concurrent-queue, which may or may not even matter: RUSTFLAGS="--cfg=loom" cargo build --no-default-features --features loom (in concurrent-queue)

Copy link
Member

@notgull notgull left a comment

Choose a reason for hiding this comment

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

Thanks!

@notgull notgull merged commit f402b7e into smol-rs:master Mar 30, 2024
10 checks passed
@notgull notgull mentioned this pull request Apr 5, 2024
notgull added a commit that referenced this pull request Apr 14, 2024
Mirrors #126

Signed-off-by: John Nunley <dev@notgull.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

[Question] Integration with loom or shuttle?
2 participants