11
\$\begingroup\$

There's a follow-up: Waiting for a lock to release with ManualResetEvent and Quartz.

I've written a simple Lock-Mechanism which is saving the states of the locks in a database. Now I need to wait for a Lock to release or timeout after a certain amount of time (blocking the main thread, this is intentional). My implementation is looking like this:

/// <summary>Waits for the lock to be released within the given timeout.</summary>
/// <param name="lockName">The name of the lock.</param>
/// <param name="timeout">The timeout in seconds.</param>
/// <returns>True if the Lock was released.</returns>
public bool waitForLock(String lockName, Int32 timeout)
{
    // IsLocked(String) does query the database for the status
    while(locker.IsLocked(lockName) && timeout > 0)
    {
        Threading.Thread.Sleep(1000);
        timeout--;
    }

    return !locker.IsLocked(lockName);
}

I always get a little bit itchy if I have to use Thread.Sleep, but I'm not sure why.

Is something wrong with this design/approach and will it bite me later? Or is there a better way to handle this?

Edit: Further clarifications: I have full control over the locker and anything affiliated with it. The main intention for this is to make sure that certain operations will not overlap, each of these operations should be taking well less then 5 seconds. The load on the server is not really an issue, because the realistic maximum number of clients simultaneous waiting for a lock could be 10 (in my case).

The blocking of the thread is not as intentional is I have stated. The best thing would be if I could keep the message pumping still active, but I fail to see how I can achieve that goal in combination with polling the database (and without the need to introduce a timer with a callback).

Edit2: The locked part is not the database, the database is only holding the status of the locks. I'm using this locking system to make sure that no operation protected by this can execute at the same time on different client machines.

\$\endgroup\$
11
\$\begingroup\$

An easier answer might be to use a ManualResetEvent. You can block the current thread for a timeout period until another thread signals the ManualResetEvent.

http://philiprieck.com/blog/archive/2004/01/27/WaitHandleSample.aspx

http://www.yoda.arachsys.com/csharp/threads/waithandles.shtml

Basically, blocking thread calls mre.Wait(), processing thread works until it calls mre.Set(). No sleeping, none of this other stuff.

An important note, always Dispose of your ManualResetEvents. Leaking handles is a bad call, this object reaches out into the COM world.

\$\endgroup\$
5
  • \$\begingroup\$ Very nice. But do you have an idea how I could poll the database in the other thread to signal something back? The only thing which comes into my mind is a simple timer which gets called x times. \$\endgroup\$ – Bobby May 27 '11 at 12:48
  • \$\begingroup\$ What you are looking for is a way to for one thread to sit and wait while another pools the db, then execute something once data is retrieved from the db? Have you considered using something like MSMQ for events instead of pooling the db? You could use an actor based model as well. What about just having the main thread pool and when it finds a new record, spin off a new thread to process it? \$\endgroup\$ – Travis May 27 '11 at 13:51
  • \$\begingroup\$ I like the idea of a separate thread, but would recommend also putting the operation dependent on the lock into the background thread rather than just the lock check. The results of the operation can be marshaled back to the main GUI thread to update the user. This would also remove the need for the ManualResetEvent. \$\endgroup\$ – Brian Reichle May 27 '11 at 23:22
  • \$\begingroup\$ @ANeves Does that help? \$\endgroup\$ – Travis Nov 24 '11 at 1:22
  • \$\begingroup\$ Quite! I tried editing, but there really wasn't more to it to improve and the system wouldn't let me correct your post with only whitespace changes. ;) [[previous comment deleted]] \$\endgroup\$ – ANeves thinks SE is evil Nov 24 '11 at 9:27
4
\$\begingroup\$

The function might block up to one second too long (if the lock is released, just after it enters Sleep).

\$\endgroup\$
1
  • \$\begingroup\$ This may or may not be noticeable depending on how it's used. (lock held for minuets with infrequent collisions or locks held for fractions of a second with frequent collisions) \$\endgroup\$ – Brian Reichle May 27 '11 at 10:26
3
\$\begingroup\$
  • 1sec may be fine, but the best choice will depend on it's usage characteristics. I recommend trying different values on a test system under expected production load.

  • Sleep does not perform standard COM and SendMessage pumping. If you are not calling this on the main GUI thread then that's fine, otherwise the application may appear unresponsive.

  • I'm a little uneasy about the use of the timeout as a loop count. Sleep is only guaranteed to not return before the timeout and may sleep longer. I suppose it's probably not likely to be a real issue though.

  • It's normal for method names in C# to be PascalCased rather than cammelCased.

I also have some points about the locker object, but I'm not sure how much control you have over it.

  • Your usage of IsLocked() seems to imply that it attempts to acquire the lock, but the name implies that it only checks that you already have it. Maybe GetLock() or AcquireLock() would be a better name.

  • Are locks released if not renewed periodically? If the application holding the lock closes it's db connection/terminates abruptly, will the lock be released in a timely fashion without intervention?

\$\endgroup\$
2
  • \$\begingroup\$ I'm aware that Sleep will block everything, but I fail to see another solution in such a use-case (see my edit). The camelCase is a typo, sorry, I'll leave it in as future reference. IsLocked() does only check for the status, it's not intended to acquire it in any way. And your note with the releasing of the locks is a good call at something I haven't spend much thought about by now. \$\endgroup\$ – Bobby May 27 '11 at 12:03
  • \$\begingroup\$ @Bobby, what do you intend to do once your method returns? If you want to acquire the lock then you would be better off with something that checks and acquires as a single atomic operation to prevent excess checks, or worse, a race condition. If you do not intend to acquire the lock it should be fine, but be aware that the lock could be re-acquired by another user by the time you leave the method. \$\endgroup\$ – Brian Reichle May 27 '11 at 23:11

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.