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

Improve ergonomics around lock guard #8

Closed
pronebird opened this issue Feb 5, 2024 · 8 comments · Fixed by #9
Closed

Improve ergonomics around lock guard #8

pronebird opened this issue Feb 5, 2024 · 8 comments · Fixed by #9

Comments

@pronebird
Copy link
Contributor

pronebird commented Feb 5, 2024

Hi,

First of all great crate, looks solid.

However I found that it becomes a bit difficult to integrate the crate because NamedLockGuard borrows from NamedLock, consider the following helper:

fn ensure_single_instance<'a>() -> anyhow::Result<NamedLockGuard<'a>> {
    let lock = NamedLock::create("my-unique-lock")?;
    lock.try_lock().map_err(|e| {
        if matches!(e, named_lock::Error::WouldBlock) {
            anyhow!("Another instance is already running.")
        } else {
            anyhow::Error::new(e).context("Couldn't acquire single instance lock.")
        }
    })
}

which doesn't compile and errors with:

16 |       lock.try_lock().map_err(|e| {
   |       ^---
   |       |
   |  _____`lock` is borrowed here
   | |
17 | |         if matches!(e, named_lock::Error::WouldBlock) {
18 | |             anyhow!("Another instance is already running.")
19 | |         } else {
20 | |             anyhow::Error::new(e).context("Couldn't acquire single instance lock.")
21 | |         }
22 | |     })
   | |______^ returns a value referencing data owned by the current function

Honestly I don't know how to untangle this and make it compile. Putting all that code into main() is not really an option for me since I don't return Result<> from main() since I don't want errors printed by default.

@pronebird
Copy link
Contributor Author

Actually I found a workaround but it's still rather cumbersome to deal with locks because lock guards borrow from them? It would be great to have an owned implementation that could be passed around without dealing with borrow checker or lifetimes.

@oblique
Copy link
Owner

oblique commented Feb 19, 2024

Internally I use parking_lot crate to workaround some other issues of the OS API. The parking_lot doesn't have owned implementation of their MutexGuard so I need to investigate a bit how I could implement this. I'm a bit busy these days but I will try to check it on my free time.

@oblique
Copy link
Owner

oblique commented Feb 19, 2024

Ok, I just found ArcMutexGuard, so it is possible to make NamedLockGuard owned.

@oblique
Copy link
Owner

oblique commented Feb 26, 2024

@pronebird I opened PR #9. Since internally NamedLock already uses Arc, I decided to break compatibility and remove lifetime instead of introducing a new type. What do you think about the changes?

@pronebird
Copy link
Contributor Author

pronebird commented Feb 27, 2024

@oblique looks good. Just pulled it into my project and it worked great. I rewrote my code a bit into two stage rocket to benefit from early return but otherwise now I can hide all the guts of named_lock initialization into separate function.

fn main() -> ExitCode {
    if run().is_ok() {
        ExitCode::SUCCESS
    } else {
        ExitCode::FAILURE
    }
}

fn run() -> Result<()> {
    let _named_lock_guard = ensure_single_instance()?;
    // [redacted]
    Ok(())
}

fn ensure_single_instance() -> named_lock::Result<named_lock::NamedLockGuard> {
    named_lock::NamedLock::create("my-unique-lock")
        .and_then(|lock| lock.try_lock())
        .inspect_err(|err| match err {
            named_lock::Error::WouldBlock => {
                eprintln!("Another instance is already running");
            }
            e => {
                eprintln!("Couldn't acquire single instance lock: {}", e);
            }
        })
}

@oblique
Copy link
Owner

oblique commented Feb 27, 2024

Thanks! I just merged the changes. I will migrate to windows-rs the next few days and then release 0.4.0.

@pronebird
Copy link
Contributor Author

@oblique let me see if I can help you with windows-rs

@pronebird
Copy link
Contributor Author

@oblique that was easy. See #10. Tests pass.

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 a pull request may close this issue.

2 participants