Skip to content

Implement OnceExt & MutexExt for parking_lot & lock_api #5044

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bschoenmaeckers
Copy link
Member

ref #5038

@bschoenmaeckers bschoenmaeckers force-pushed the lockext_parking_lot branch 3 times, most recently from 39cba21 to 3e76d58 Compare April 5, 2025 15:51
@bschoenmaeckers bschoenmaeckers marked this pull request as ready for review April 7, 2025 08:07
@bschoenmaeckers
Copy link
Member Author

In my first implementation I used associated types (generic over some lifetime) for the return value of OnceExt & MutexExt. But this did not work because our MSRV. So I moved the types into a additional type parameter, which looks less clean unfortunately.

@ngoldbaum
Copy link
Contributor

Can we get a mention in this docs section that we also have MutexExt implementations for third-party synchronization libraries behind a cargo feature.

Copy link
Contributor

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

Also on second look, is this missing tests?

@@ -0,0 +1 @@
Implement `OnceExt` & `MutexExt` for `parking_lot` & `lock_api`
Copy link
Contributor

Choose a reason for hiding this comment

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

You should also mention the new features IMO

@@ -1047,6 +1132,51 @@ mod tests {
});
}

#[cfg(feature = "macros")]
#[cfg(all(feature = "parking_lot", not(target_arch = "wasm32")))] // We are building wasm Python with pthreads disabled
#[test]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we get a test for the lock_api feature too?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The lock_api crate does not provide actual locks, only the traits. So we need parking_lot to test it.

Copy link
Contributor

Choose a reason for hiding this comment

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

But can't we turn on both features and write a test that uses e.g. lock_api::RawMutex::lock_once_py_attached?

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean lock_api::Mutex::<parking_lot::RawMutex, _>::lock_py_attached?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for speaking imprecisely - I'm not very familiar with parking_lot. I actually didn't realize until now that lock_api is provided by parking_lot.

What I'm trying to understand is why it's not possible to write a test that enables both features and uses the functionality provided by the lock_api feature. When I asked before, you explained that the reason is because lock_api only provides traits. I'm trying to say that, at least from my point of view, that doesn't prevent writing a test that uses a parking_lot mutex and turns on the lock_api feature. Can you explain where I'm misunderstanding things?

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