Skip to content

Support blocking and non-blocking operations on the same Mutex #25

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

Closed
wants to merge 1 commit into from

Conversation

joshtriplett
Copy link

Add lock_blocking and lock_arc_blocking methods to allow sharing the
same lock between blocking and non-blocking code.

Introduce a helper macro to avoid duplicating the code of the
acquire_slow method.

@zeenix
Copy link
Member

zeenix commented Jul 2, 2022

Thanks @joshtriplett ! LGTM. Could see check why the mini test is failing?

@zeenix
Copy link
Member

zeenix commented Jul 2, 2022

Thanks @joshtriplett ! LGTM. Could see check why the mini test is failing?

Ah nm. @taiki-e already fixed it: #26 . Can you rebase?

@notgull
Copy link
Member

notgull commented Jul 3, 2022

I'm a bit concerned about this. If this crate moves to a different backing implementation of synchronization, this may include a higher burden of porting.

Add `lock_blocking` and `lock_arc_blocking` methods to allow sharing the
same lock between blocking and non-blocking code.

Introduce a helper macro to avoid duplicating the code of the
`acquire_slow` method.
@joshtriplett
Copy link
Author

@zeenix Rebased.

@notgull Support for both blocking and non-blocking seems fairly fundamental to event-listener, and I think it's an important primitive for bridging between sync and async code. This is the kind of thing I'm hoping to see in the standard library in the future: not two completely independent lock types, but one lock type that works in both sync and async code.

@notgull
Copy link
Member

notgull commented Jul 3, 2022

I see. I wonder if this could be done for async-channel, since that is also backed by event-listener.

@joshtriplett
Copy link
Author

@notgull I very much hope so!

@zeenix
Copy link
Member

zeenix commented Jul 6, 2022

@smol-rs/admins so any objections in merging this?

Comment on lines +102 to +115
match self
.state
.compare_exchange(0, 1, Ordering::Acquire, Ordering::Acquire)
.unwrap_or_else(|x| x)
{
// Lock acquired!
0 => return,

// Lock is held and nobody is starved.
1 => {}

// Somebody is starved.
_ => break,
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we factor this block of code into a separate method? It is repeated 2 times in this method with small variations in the _=> case, and with this PR applied would then be duplicated again, resulting in 5 repetitions...

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure we can factor that into a method, since it includes control flow. I could make it a macro if you like.

Copy link
Member

Choose a reason for hiding this comment

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

since it includes control flow

that can be factored out, using something like the core::ops::ControlFlow, e.g.:

enum ScxResult {
    /// Lock acquired
    Acquired,
    /// Lock is held and nobody is starved
    HeldUnstarved,
    /// Somebody is starved
    Starved,
}
fn do_scx(state: &...) -> ScxResult {
    use ScxResult::*;
    match state
        .compare_exchange(0, 1, Ordering::Acquire, Ordering::Acquire)
        .unwrap_or_else(|x| x)
    {
        0 => Acquired,
        1 => HeldUnstarved,
        _ => Starved,
    }
}

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we could refactor this into a trait, like I did for the OnceCell in #27.

@taiki-e
Copy link
Collaborator

taiki-e commented Jul 7, 2022

I think a caveat like "calling this method on async code may cause deadlock" would be nice. Otherwise LGTM.

I wonder if this could be done for async-channel, since that is also backed by event-listener.

I would accept a PR that does this.

@dignifiedquire
Copy link

What's needed for getting this in?

It would also be super useful to have this for RwLock.

@notgull
Copy link
Member

notgull commented Mar 2, 2023

What's needed for getting this in?

It would also be super useful to have this for RwLock.

It would need a rebase, among other things. I've opened #39 as a replacement that also implements blocking ops for the other lock types.

@notgull
Copy link
Member

notgull commented Sep 23, 2023

Closed by #56

@notgull notgull closed this Sep 23, 2023
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.

6 participants