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(maitake-sync): add proper ScopedRawMutex bound to wait_map::Wait #515

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

peterkrull
Copy link
Contributor

I was unable to await the Wait when using a mutex defined in the mutex crate because a trait bound was not met. The current trait impl is not general enough.

@peterkrull
Copy link
Contributor Author

This seems to also be a thing for other types which specifically use the DefaultMutex default type, like the Lock for the async mutex.

pub struct Lock<'a, T: ?Sized, L: ScopedRawMutex = DefaultMutex> { .. }

impl<'a, T> Future for Lock<'a, T> {..}

Copy link
Owner

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

Whoops! Thanks for fixing this!

We should probably add some tests that actually construct each synchronization primitive with an overridden ScopedRawMutex type and check that all the expected trait impls are present, to prevent against future regressions with this sort of thing. I don't think it's worth holding up merging this fix for such tests, but I'll open an issue because I think it's worth doing.

@hawkw
Copy link
Owner

hawkw commented Feb 6, 2025

Opened #516 for adding tests to ensure expected Future impls are present.

@hawkw hawkw enabled auto-merge (squash) February 6, 2025 17:40
hawkw added a commit that referenced this pull request Feb 6, 2025
…dRawMutex`

This commit fixes the trait bounds on the `mutex::Lock` type's `Future`
implementation so that it's present when used with an overridden
`ScopedRawMutex`, as well as the default `DefaultMutex`. Thanks to
@peterkrull for reporting this issue in [this comment][1]!

As noted in #516, I've also added tests for this, which wouldn't have
compiled prior to this change.

[1]: #515 (comment)
@hawkw
Copy link
Owner

hawkw commented Feb 6, 2025

This seems to also be a thing for other types which specifically use the DefaultMutex default type, like the Lock for the async mutex.

pub struct Lock<'a, T: ?Sized, L: ScopedRawMutex = DefaultMutex> { .. }

impl<'a, T> Future for Lock<'a, T> {..}

#517 fixes this for the mutex::Lock future. Checking the other async synchronization primitives (WaitQueue, RwLock, and Semaphore), it looks like their Future types already have proper trait bounds, but I'll add regression tests for them as well.

@hawkw hawkw disabled auto-merge February 6, 2025 18:09
@hawkw hawkw enabled auto-merge (rebase) February 6, 2025 18:09
@hawkw hawkw disabled auto-merge February 6, 2025 18:09
@hawkw hawkw merged commit 509a8b7 into hawkw:main Feb 6, 2025
17 checks passed
hawkw added a commit that referenced this pull request Feb 6, 2025
…dRawMutex`

This commit fixes the trait bounds on the `mutex::Lock` type's `Future`
implementation so that it's present when used with an overridden
`ScopedRawMutex`, as well as the default `DefaultMutex`. Thanks to
@peterkrull for reporting this issue in [this comment][1]!

As noted in #516, I've also added tests for this, which wouldn't have
compiled prior to this change.

[1]: #515 (comment)
hawkw added a commit that referenced this pull request Feb 6, 2025
…edRawMutex` (#517)

This commit fixes the trait bounds on the `mutex::Lock` type's `Future`
implementation so that it's present when used with an overridden
`ScopedRawMutex`, as well as the default `DefaultMutex`. Thanks to
@peterkrull for reporting this issue in [this comment][1]!

As noted in #516, I've also added tests for this, which wouldn't have
compiled prior to this change.

[1]: #515 (comment)
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