All threads only in one method at a time?

Question

I have several objects inheriting from ClassA, which has an abstract method MethodA.

Each of these inheriting objects can allow up to a specific number of threads simutaneously into their MethodA.

The catch: Threads can only be in an object's MethodA, while no other objects' MethodA is being processed at the same time.

How can I solve this? I am thinking about using a semaphore, but don't know exactly how to do it, because I just can't wrap my head around the problem enough to get a solution.

EDIT:

Example code (may contain syntax errors:)

public class ClassA
{
  public virtual void MethodA{}
}

public class OneOfMySubclassesOfClassA // there can be multiple instances of each subclass!
{
  public override void MethodA{

  // WHILE any number of threads are in here, THIS MethodA shall be the ONLY MethodA in the entire program to contain threads
  EDIT2: // I mean: ...ONLY MethodA of a subclass (not of a instance of a subclass) in the entire program...
}
}

...and more subclasses...

Syllogistic

I suggest you post some code that will let people assist you. This really is a very vague question. – Associationism

if I get you correctly, you want to fire multiple threads doing the work of MethodA, but only once the previous thread has finished? – Rutty

May look at the semaphore class: msdn.microsoft.com/en-us/library/… – Pawsner

@FlorisPrijt my understanding is he wants to allow a group of X threads into MethodA but only one group of X threads at a time – Symbology

if you want to control some aspect of MethodA, you should not have it as an abstract method – Mcconnell

Is there only one instance of each subclass shared by all threads or is it possible that several threads are executing the method of different instance of the same subclass? – Galacto

I'll rephase my post: I want threads only in 1 methodA (methodA is inherited by multiple objects) at any given time. – Syllogistic

There are several methods MethodA on different derived types - ClassA1.MethodA, ClassA2.MethodA, ClassA3.MethodA and so on. At any point in time threads are only allowed to execute one of this methods. Four threads in method ClassA2.MethodA is valid, but not one thread executing ClassA1.MethodA and three threads executing ClassA2.MethodA. – Galacto

@ Daniel Brückner that is correct. I put it shorter in the reponse above your comment. so it seems I need a semaphore? or multiple semaphore? – Syllogistic

The exact structure depends on the number of instances - one instance per thread, one instance shared by all threads, a mix of that, ... – Galacto

@ Daniel Brückner EDIT: is IS possible that several threads are executing the method of different instances of the same subclass (fyi: the subclasses are asp controllers inheriting from a base controller class. And I believe asp creates an instance of a controller for each http request it recieves, so yeah..) @Mcconnell does not need to be abstract method necessarily. – Syllogistic

This is a very strange and therefore probably artificial request. Locking is needed around data, not around methods. So the core of this question makes little sense. – Debutant

Answer 1

The derived type is used as type argument in the base class together with a static semaphore to get one semaphore shared between all instances of each subclass. And then there is some mess to ensure that only one type is active. A quick test indicates that this works correctly but there is an issue.

Assume for example that the method of ClassA1 is currently executing. If new request to execute this methods arrive with high frequency it may happen that other derived classes get no chance to execute because there are constantly new threads executing the method of class ClassA1.

internal abstract class ClassA<TDerived> : ClassA
{
    private const Int32 MaximumNumberConcurrentThreads = 3;

    private static readonly Semaphore semaphore = new Semaphore(ClassA<TDerived>.MaximumNumberConcurrentThreads, ClassA<TDerived>.MaximumNumberConcurrentThreads);

    internal void MethodA()
    {
        lock (ClassA.setCurrentlyExcutingTypeLock)
        {
            while (!((ClassA.currentlyExcutingType == null) || (ClassA.currentlyExcutingType == typeof(TDerived))))
            {
                Monitor.Wait(ClassA.setCurrentlyExcutingTypeLock);
            }

            if (ClassA.currentlyExcutingType == null)
            {
                ClassA.currentlyExcutingType = typeof(TDerived);
            }

            ClassA.numberCurrentlyPossiblyExecutingThreads++;

            Monitor.PulseAll(ClassA.setCurrentlyExcutingTypeLock);
        }

        try
        {
            ClassA<TDerived>.semaphore.WaitOne();

            this.MethodACore();
        }
        finally
        {
            ClassA<TDerived>.semaphore.Release();
        }

        lock (ClassA.setCurrentlyExcutingTypeLock)
        {
            ClassA.numberCurrentlyPossiblyExecutingThreads--;

            if (ClassA.numberCurrentlyPossiblyExecutingThreads == 0)
            {
                ClassA.currentlyExcutingType = null;

                Monitor.Pulse(ClassA.setCurrentlyExcutingTypeLock);
            }
        }
    }

    protected abstract void MethodACore();
}

Note that a wrapper method is used to call the actual implementation in MethodACore. All the synchronization objects shared between all derived classes are in a non-generic base class.

internal abstract class ClassA
{
    protected static Type currentlyExcutingType = null;

    protected static readonly Object setCurrentlyExcutingTypeLock = new Object();

    protected static Int32 numberCurrentlyPossiblyExecutingThreads = 0;
}

The derived classes will look like this.

internal sealed class ClassA1 : ClassA<ClassA1>
{
    protected override void MethodACore()
    {
        // Do work here.
    }
}

internal sealed class ClassA2 : ClassA<ClassA2>
{
    protected override void MethodACore()
    {
        // Do work here.
    }
}

Unfortunately I have no time to explain how and why this works in more detail right now but I will update the answer tomorrow.

Juncaceous

looks very promising looking forward for the whole thing :) (need the part where it checks if the semaphores of the other subclasses are empty so it know whether it can do waitone() at all) – Syllogistic

Thank you very much for the full version, I bet that took some work. Just 2 questions though. Shouldn't the derived classes call MethodA instead of MethodACore? And shouldn't MethodA be virtual? Thanks again, I'll implement it when I get home tonight. – Syllogistic

Yes, that took some time. :D ClassA.MethodA does the heavy locking stuff and calls the abstract (and therefore implicitly virtual) method ClassA.MethodACore to do the actual work. This method is in turn overridden in the derived classes to implement the specific task for each derived class. – Galacto

Keep in mind that this can be easily broken. Imagine MeanClass : ClassA<ClassA2>. There's no restriction that the type that's in the generic parameter is the type of the instance itself. – Chessa

That's true, of course. This whole mess is far from perfect. I really did not expect that this questions turns out this way. I strongly believe there is a far better solution than mine. – Galacto

OK, I'm here to report everything works fine with high frequency http requests sent to my controllers. I needed this mechanism because of a client unit test of mine that deletes all entites on the server that were previously submitted to the client, on entity by entity basis (highly multithreaded too). since my server uses nhibernate and has cascade.delete references, the transactions of the high frequency deletes were sometimes conflicting if entity types were deleted in random sequences while the deletes were also overlapping. o well, maybe somebody will understand :) nevermind :) – Syllogistic

Answer 2

public abstract class Foo
{
    private static Type lockedType = null;
    private static object key = new object();
    private static ManualResetEvent signal = new ManualResetEvent(false);
    private static int threadsInMethodA = 0;
    private static Semaphore semaphore = new Semaphore(5, 5);//TODO set appropriate number of instances

    public void MethodA()
    {
        lock (key)
        {
            while (lockedType != this.GetType())
            {
                if (lockedType == null)
                {
                    lockedType = this.GetType();
                    //there may be other threads trying to get into the instance we just locked in
                    signal.Set();
                }
                else if (lockedType != this.GetType())
                {
                    signal.WaitOne();
                }
            }

            Interlocked.Increment(ref threadsInMethodA);
        }
        semaphore.WaitOne();

        try
        {
            MethodAImplementation();
        }
        finally
        {
            lock (key)
            {
                semaphore.Release();
                int threads = Interlocked.Decrement(ref threadsInMethodA);
                if (threads == 0)
                {
                    lockedType = null;
                    signal.Reset();
                }
            }
        }
    }

    protected abstract void MethodAImplementation();
}

So there are a few key points here. First off, we have a static object that represents the only instance that is allowed to have threads. null means the next thread to come along can put in "their" instance. If another instance is the "active" one the current thread waits on the manual reset event until either there is no locked instance, or the locked instance changed to what might possibly be that thread's instance.

It's also important to count the number of threads in the method to know when to set the locked instance to null (setting to to null without keeping track of that would let new instances start while a few of the previous instances were finishing.

Locks around another key at the start and end are rather important.

Also beware that with this setup it's possible for one type to starve out other types, so if this is a heavily contended resource it's something to watch out for.

Chessa

Shouldn't the entire thing be in a try/finally to unsure proper incrementing and decrementing of threadsInMethodA – Fluent

Cleaner than my solution: +1 – Lever

This implementation does not allow concurrent execution of different instances of the same derived type. It does also not limit the maximum number of concurrently executing threads. – Galacto

@DanielBrückner Edited; neither is a particularly significant change; given what's all already in place. – Chessa

Now it will easily deadlock - if a thread has to wait when acquiring the semaphore it will hold the lock on key while waiting but this lock is required by any thread wanting to release the semaphore. – Galacto

@DanielBrückner if a thread has to wait when acquiring the semaphore it will hold the lock on key while waiting That is incorrect. It will release the lock while it's waiting. You could move the semaphore wait until after the end of the lock block though; it wouldn't break the implementation. – Chessa

No, the lock on key will not be released, it is completely unrelated with the semaphore. But moving the wait on the semaphore outside of the lock will indeed (probably) solve the issue. – Galacto

I also just noticed that you never signal the manual reset event; one of the calls to Reset is probably intended to by a call to Set. – Galacto

Unnaturally I have to say, that there is another issue - something similar as stated for waiting on the semaphore holds for waiting on the manual reset event. The first thread waiting on the manual reset event will keep the lock on key and prevent other threads from executing even if they are trying to execute instances of the type matching lockedType. – Galacto

Answer 3

Assuming you have a list of all relevant instances, this code will lock on all other instances and thus allow only one instance's code to be executed at any given time:

void MethodA()
{
     foreach (var obj in objects)
         if (obj != this)
             Monitor.Enter(obj);

try
{
    // do stuff
}
finally
{
        foreach( var obj in objects)
        if (obj != this)
                 Monitor.Exit(obj);
}   
}

Lever

This code could very easily deadlock. You need to ensure that your "lock all the things" is logically atomic, which means adding a separate mutex that you lock on when locking on all of the items in the collection. – Chessa

Also, where is the objects collection going to come from? How will he get a hold of all instances of his abstract class? – Chessa

@Chessa 1. I don't think it will deadlock, as the locks are always aquired in the same order. 2. he would have to create this list somehow, e.g. insert each object upon construction. – Lever

Could I not create a static field e.g. in the abstract class that holds all instances of all subclasses? Surely I could populate and depopulate that list through the base class constructor each time a subclass get's created or destroyed – Syllogistic