CODEWITHSUNDEEP
c#thread-safety
Umesh Navani
Umesh Navani

Reputation: 55

Monitor.TryEnter for multiple resources

I tried searching for this but did not find the suggestion best suited for the issue that I am facing.

My issue is that we have list/stack of available resources (Calculation Engines). These resources are used to perform certain calculation.

The request to perform the calculation is triggered from an external process. So when the request for calculation is made, I need to check if any of the available resources are currently not performing other calculations, If so wait for some time and check again.

I was wondering what the best way to implement this is. I have the following code in place, but not sure if it is very safe.

If you have any further suggestions, that will be great:

void Process(int retries = 0) {
    CalcEngineConnection connection = null;
    bool securedConnection = false;
    foreach (var calcEngineConnection in _connections) {
        securedConnection = Monitor.TryEnter(calcEngineConnection);
        if (securedConnection) {
            connection = calcEngineConnection;
            break;
        }
    }
    if (securedConnection) {
        //Dequeue the next request
        var calcEnginePool = _pendingPool.Dequeue();

        //Perform the operation and exit.
        connection.RunCalc(calcEnginePool);
        Monitor.Exit(connection);
    }
    else {
        if (retries < 10)
            retries += 1;
        Thread.Sleep(200);
        Process(retries);
    }
}

Upvotes: 3

Views: 484

Answers (2)

Kiril
Kiril

Reputation: 40345

There are multiple issues that could potentially occur, but let's simplify your code first:

void Process(int retries = 0) 
{
    foreach (var connection in _connections) 
    {
        if(Monitor.TryEnter(connection))
        {
            try
            {
                //Dequeue the next request
                var calcEnginePool = _pendingPool.Dequeue();

                //Perform the operation and exit.
                connection.RunCalc(calcEnginePool);
            }
            finally
            {
                // Release the lock
                Monitor.Exit(connection);
            }
            return;
        }
    }

    if (retries < 10)
    {
        Thread.Sleep(200);
        Process(retries+1);
    }
}

This will correctly protect your connection, but note that one of the assumptions here is that your _connections list is safe and it will not be modified by another thread.

Furthermore, you might want to use a thread safe queue for the _connections because at certain load levels you might end up using only the first few connections (not sure if that will make a difference). In order to use all of your connections relatively evenly, I would place them in a queue and dequeue them. This will also guarantee that no two threads are using the same connection and you don't have to use the Monitor.TryEnter().

Upvotes: 1

Jon Skeet
Jon Skeet

Reputation: 1500893

I'm not sure that using Monitor is the best approach here anyway, but if you do decide to go that route, I'd refactor the above code to:

bool TryProcessWithRetries(int retries) {
    for (int attempt = 0; attempt < retries; attempt++) {
        if (TryProcess()) {
            return true;
        }
        Thread.Sleep(200);
    }
    // Throw an exception here instead?
    return false;
}

bool TryProcess() {
    foreach (var connection in _connections) {
        if (TryProcess(connection)) {
            return true;
        }
    }
    return false;
}

bool TryProcess(CalcEngineConnection connection) {
    if (!Monitor.TryEnter(connection)) {
        return false;
    }
    try {
        var calcEnginePool = _pendingPool.Dequeue();
        connection.RunCalc(calcEnginePool);
    } finally {
        Monitor.Exit(connection);
    }
    return true;
}

This decomposes the three pieces of logic:

  • Retrying several times
  • Trying each connection in a collection
  • Trying a single connection

It also avoids using recursion for the sake of it, and puts the Monitor.Exit call into a finally block, which it absolutely should be in.

You could replace the middle method implementation with:

return _connections.Any(TryProcess);

... but that may be a little too "clever" for its own good.

Personally I'd be tempted to move TryProcess into CalcEngineConnection itself - that way this code doesn't need to know about whether or not the connection is able to process something - it's up to the object itself. It means you can avoid having publicly visible locks, and also it would be flexible if some resources could (say) process two requests at a time in the future.

Upvotes: 2

Related Questions