tags:

views:

1433

answers:

12

Is there any way to lock on an integer in C#, integers can not be used with lock because they are boxed (and lock only locks on references).

The scenario is as follows, I have that forum based website which support moderation, I want to make sure that no more than one moderator can moderate a post at a time, for this I want to use the ID of the post.

I've had a couple of ideas so far (e.g. using a dictionary<int, object>), but I'm looking for a better and cleaner way.

Any suggestions?

+1  A: 

Why don't you lock on the whole posting instead just on its ID?

Konrad Rudolph
That assumes he has some posting class he can lock (which would be nice OO) but it could be that the id is just an input param for DB data, at which point I guess the lock should be at the DB level, so circularly if you can lock at all, do it against the posting instance :)
annakata
Nice answer annakata, you saved me the time :) .. it's just a param as you indicated
Waleed Eissa
If loaded from the DB, they will have different references so I can't lock on this
Waleed Eissa
Given your scenario, I'd use Greg's suggestion. That said, you could always wrap the loading of your records so that for any given posting only one object is ever created and subsequently reused (this works by keeping a local cache of objects in a dictionary).
Konrad Rudolph
+6  A: 

If it's a website then using an in-process lock probably isn't the best approach as if you need to scale the site out onto multiple servers, or add another site hosting an API (or anything else that would require another process accessing the same data to exist) then all your locking strategies are immediately ineffective.

I'd be inclined to look into database-based locking for this. The simplest approach is to use optimistic locking with something like a timestamp of when the post was last updated, and to reject updates made to a post unless the timestamps match.

Greg Beech
Actually I thought about this but I'm doing this temporarily, if the site is to run on a server farm anytime, I'll use some distributed locking mechanism then.
Waleed Eissa
Just to add, on the database side it's an sproc which updates multiple tables (MyISAM tables in MySQL), I still haven't figured out how to block other requests trying to execute the same sproc until it finishes, not sure how this can be done or even if it can be done at all.
Waleed Eissa
Not sure how to do this type of thing in MySQL I'm afraid. In SQL Server you could use sp_getapplock so there may be something similar in MySQL to this. Alternatively, don't block the requests, just allow them and raise an error if the timestamps don't match.
Greg Beech
A: 

You should use a sync object like this:

public class YourForm
{
    private static object syncObject = new object();

    public void Moderate()
    {
     lock(syncObject)
     {
      // do your business
     }
    }
}

But this approach shouldn't be used in a web app scenario.

Rodrigo Guerreiro
A: 

hmm, maybe you could create a list of the posts currently being edited and check that?

Botz3000
Actually this is what I meant when I mentioned using a dictionary, but I believe this can get messy very easily
Waleed Eissa
A: 

I doubt you should use a database or O/S level feature such as locks for a business level decision. Locks incur significant overheads when held for long times (and in these contexts, anything beyond a couple of hundred milliseconds is an eternity).

Add a status field to the post. If you deal with several therads directly, then you can use O/S level locks -- to set the flag.

Pontus Gagge
+2  A: 

I would personally go with either Greg's or Konrad's approach.

If you really do want to lock against the post ID itself (and assuming that your code will only ever be running in a single process) then something like this isn't too dirty:

public class ModeratorUtils
{
    private static readonly HashSet<int> _LockedPosts = new HashSet<int>();

    public void ModeratePost(int postId)
    {
        bool lockedByMe = false;
        try
        {
            lock (_LockedPosts)
            {
                lockedByMe = _LockedPosts.Add(postId);
            }

            if (lockedByMe)
            {
                // do your editing
            }
            else
            {
                // sorry, can't edit at this time
            }
        }
        finally
        {
            if (lockedByMe)
            {
                lock (_LockedPosts)
                {
                    _LockedPosts.Remove(postId);
                }
            }
        }
    }
}
LukeH
This needs try/finally to be robust.
Joe
@Joe, Good point, updated.
LukeH
+7  A: 

I like doing it like this

public class Synchronizer {
    private Dictionary<int, object> locks;
    private object myLock;

    public Synchronizer() {
        locks = new Dictionary<int, object>();
        myLock = new object();
    }

    public object this[int index] {
        get {
            lock (myLock) {
                object result;
                if (locks.TryGetValue(index, out result))
                    return result;

                result = new object();
                locks[index] = result;
                return result;
            }
        }
    }
}

Then, to lock on an int you simply (using the same synchronizer every time)

lock (sync[15]) { ... }

This class returns the same lock object when given the same index twice. When a new index comes, it create an object, returning it, and stores it in the dictionary for next times.

It can easily be changed to work generically with any struct or value type, or to be static so that the synchronizer object does not have to be passed around.

configurator
If you make it static, keep in mind indexers in C# can't declared as static, so just make it a method instead, worked great for me, thanks Configurator
Myster
+1  A: 

You need a whole different approach to this.

Remember that with a website, you don't actually have a live running application on the other side that responds to what the user does.

You basically start a mini-app, which returns the web-page, and then the server is done. That the user ends up sending some data back is a by-product, not a guarantee.

So, you need to lock to persist after the application has returned the moderation page back to the moderator, and then release it when the moderator is done.

And you need to handle some kind of timeout, what if the moderator closes his browser after getting the moderation page back, and thus never communicates back with the server that he/she is done with the moderation process for that post.

Lasse V. Karlsen
Note sure what this means, it's a static method so I sure can use locking inside it e.g.public static void ModeratePost(int postID, ModerationAction action) { // lock on the post id }It's called from a webservice as the moderation system uses Ajax
Waleed Eissa
So you only need to lock as part of the webservice call, and unlock before you return? In that case, a simple database lock would suffice, wouldn't it?
Lasse V. Karlsen
In that method, ModeratePost(), I delete the post (in some database) and I execute an sproc in another database, I don't want any other thread to execute that sproc while it's still running, after it finishes it will have deleted all the records that are related to that action, so if another thread executes the sproc it won't have any records to work on and it will just return, but if it executes the sproc while it's still running, this could affect the integrity of the data ...
Waleed Eissa
A: 

Ideally you can avoid all the complex and brittle C# locking and replace it with database locking, if your transactions are designed correctly then you should be able to get by with DB transactions only.

Sam Saffron
Unfortunately I'm using MySQL with MyISAM tables, so no transactions
Waleed Eissa
My concern is, what happens when you scale and have multiple app domains running against a single DB, how are you going to manage access to the DB (who know maybe YAGNI)
Sam Saffron
I will probably get rid of MySQL then :) .. but seriously, I have an sproc for moderation that inserts records into tables and deletes records from other tables, as far as I understand (and please correct me if I'm wrong, also forgive me my poor understanding), transaction are used to ensure that all the statements will be executed or none of them will, I want something that will block other clients until the transaction finishes, can this be done with transactions?
Waleed Eissa
The point is that the details of the post to be moderated exist in some table, there are other tables that contain the reporting details .. etc, the sproc deletes all this and inserts them into other tables (history tables, just used for archiving), when the sproc finishes there will be nothing to do (all records are deleted), so if I can manage to block all other clients until the sproc finishes they will have nothing to do and just return, this is exactly what I'm trying to achieve ..
Waleed Eissa
A: 

Two boxed integers that happen to have the same value are completely indepent objects. So if you wanted to do this, your idea of Dictionary would probably be the way to go. You'd need to synchronize access to the dictionary to make sure you are always getting the same instance. And you'd have the problem of the dictionary growing in size.

Joe
A: 

C# locking is for thread safety and doesn't work the way you want it to for web applications.

The simplest solution is adding a column to the table that you want to lock and when somone locks it write to the db that that column is locked.

Dont let anyone open a post in edit mode if the column is locked for editing.

Otherwise maintain a static list of locked entry Ids and compare to that before allowing an edit.

Omar Kooheji
hmmm, I don't see why not, two requests mean two different threads, I don't see a problem with that .. are you saying that lock is never used in web applications?
Waleed Eissa
If you do this, make sure your read-lock and add-lock-flag is one atomic action.
Myster
A: 

You want to make sure that a delete doesn't happen twice?

CREATE PROCEDURE RemovePost( @postID int )
AS
    if exists(select postID from Posts where postID = @postID)
    BEGIN
        DELETE FROM Posts where postID = @postID
        -- Do other stuff
    END

This is pretty much SQL server syntax, I'm not familiar with MyISAM. But it allows stored procedures. I'm guessing you can mock up a similar procedure.

Anyhow, this will work for the majority of cases. The only time it will fail is if two moderators submit at almost exactly the same time, and the exists() function passes on one request just before the DELETE statement executes on another request. I would happily use this for a small site. You could take it a step further and check that the delete actually deleted a row before continuing with the rest, which would guarantee the atomicity of it all.

Trying to create a lock in code, for this use case, I consider very impractical. You lose nothing by having two moderators attempting to delete a post, with one succeeding, and the other having no effect.

Josh Smeaton
Thanks Josh but actually this is not exactly what I'm trying to do. If it was only a delete operation there wouldn't be a problem because the table is locked anyway by MySQL while deleting (any operation that changes the data in a table - ie. DELETE, UPDATE and INSERT - is atomic in MySQL and every other RDBMS). I have a set of related operations that updates, deletes and inserts in multiple tables and this is why I'm trying to use locking, actually there's a statement in MySQL for locking (it also supports time limited locks) but I'm trying to avoid doing it this way.
Waleed Eissa
It's hard to explain all the details in here and this is why my question was only about what I finally want to accomplish.
Waleed Eissa