3

I need to check whether a file is locked using boost::interprocess::file_lock. I produced this, but I'm worried what it's gonna do:

bool DataCache::isLocked() const {
    bool res = lock_->try_lock();
    if(res)
        lock_->unlock();
    return res;
}

Is it a good idea? Isn't there a way to check it without locking it?

Tomáš Zato
  • 50,171
  • 52
  • 268
  • 778
  • 7
    You realize this information is stale as soon as you unlock the lock. It's the same problem as checking if a file exists without opening it. The status can change immediately after your check. Don't base critical decisions on the results or you will introduce subtle race conditions. – John Kugelman Nov 04 '15 at 14:45
  • @JohnKugelman Good point. The answer is no then, I guess, only valid way is to lock and do the stuff. – Tomáš Zato Nov 04 '15 at 14:50
  • You create a interface functions for `tryLock` and `unlock` and use these? – Simon Kraemer Nov 04 '15 at 14:56
  • @sehe: Posted what I meant as answer – Simon Kraemer Nov 04 '15 at 15:36
  • @SimonKraemer Posted [my response](http://stackoverflow.com/a/33526994/85371) to that – sehe Nov 04 '15 at 16:27

2 Answers2

9

While the other answer is the key to not introducing a race condition, there is no reason to drop exception safety and error-resilience that comes from using the proper RAII wrappers like std::lock_guard<> and std::unique_lock<>.

You'd want to write:

if (auto lk = try_to_lock(mx)) {
    std::cout << "simple test\n";
} // automatically unlocks at scope exit

And you can. Here's my simple implementation:

template <typename Lockable>
std::unique_lock<Lockable> try_to_lock(Lockable& lockable) {
    return std::unique_lock<Lockable> (lockable, std::try_to_lock);
}

Live On Coliru

#include <mutex>
#include <iostream>

int main() {
    // demo
    std::mutex mx;

    if (auto lk = try_to_lock(mx)) {
        std::cout << "simple test\n";
    } // automatically unlocks at scope exit

    if (auto lk = try_to_lock(mx)) {
        std::cout << "yes\n";

        if (auto lk = try_to_lock(mx)) {
            std::cout << "oops?!\n"; // not reached
        } else {
            std::cout << "no recursive lock\n";
        }

        // but you can manipulate the lock if you insist:
        lk.unlock();

        if (auto lk = try_to_lock(mx)) {
            std::cout << "now we can lock again\n";
        } else {
            std::cout << "oops?!\n"; // not reached
        }
    }
}

Prints:

simple test
yes
no recursive lock
now we can lock again
sehe
  • 374,641
  • 47
  • 450
  • 633
  • PS. note that `unique_lock` is movable so you can make `try_lock_guard` trivially movable (will do when I come back) – sehe Nov 04 '15 at 16:28
  • 2
    +1 as RAII should be the preferred way. I didn't test it, but shouldn't `unique_lock` in combination `std::try_to_lock` do pretty much the same without a wrapper? `if (auto lk = std::unique_lock(mx, std::try_to_lock)){...}`. This should also work if the `DataCache` provides the functions `lock`, `unlock` and `try_lock` – Simon Kraemer Nov 04 '15 at 16:43
  • Oh hey. `try_to_lock` was new to me. Even more baffling, the explicit `bool`-conversion is also already there. Simplified my answer :) – sehe Nov 04 '15 at 16:51
  • We all learn something new everyday. I was also pretty surprised finding this. :-) – Simon Kraemer Nov 04 '15 at 16:52
0

As this doesn't fit in a comment: You could create "interface functions" for tryLock and unlock externally.

e.g.:

bool DataCache::try_lock() const {
    return lock_->try_lock();
}

void DataCache::unlock() const {
    lock_->unlock();
}

Usage:

DataCache cache;
if(cache.try_lock())
{
    cache.doSomething();
    cache.unlock();
}
else
{
    //....
}

I'm not sure if the const will work here. I just copied it from the question code.

Simon Kraemer
  • 5,700
  • 1
  • 19
  • 49
  • Yeah, this would be correct approach. The actual thing is that in my case, I realized I don't even know what was `locked` method meant for, but your answer is correct for anyone who will be about to do this. – Tomáš Zato Nov 04 '15 at 15:38
  • Oh well. If this answers the question I'd suggest an idiom like `if (auto lk = try_lock(cache)) { .... }` with RAII. Will post in a while (busy with kids) – sehe Nov 04 '15 at 15:51