[Proposal] Add catch block to lock block

[rev3r]

Background

The lock block can lead to inconsistent state, as Eric Lippert wrote:

The purpose of the lock statement is to help you protect the integrity of a mutable resource that is shared by multiple threads. But suppose an exception is thrown halfway through a mutation of the locked resource. Our implementation of lock does not magically roll back the mutation to its pristine state, and it does not complete the mutation. Rather, control immediately branches to the finally, releasing the lock and allowing every other thread that is patiently waiting to immediately view the messed-up partially mutated state! If that state has privacy, security, or human life and safety implications, the result could be very bad indeed.

Similar opinion was also expressed by C# async concept designer Jeffrey Richter.

Proposal

I suggest adding optional catch block to lock block. That will allow developers to handle exceptions and roll back data if necessary.

lock (_obj)
{
    //code that needs to be synchronized
}
catch (Exception e)
{
    //rolling back changes in case of error
}

Alternative

Now developers needs to add yet another try block (because lock is internally a try-finally block) to handle those situations:

lock (_obj)
{
    try
    {
        //code that needs to be synchronized
    }
    catch (Exception e)
    {
        //rolling back changes in case of error
    }
}

That is less than ideal and looks ugly.

[HaloFour]

This would be equivalent and works today. Although note that as with your code the lock would be released by the time you get to the catch clause.

lock (_obj) try
{
    //code that needs to be synchronized
}
catch (Exception e)
{
    //rolling back changes in case of error
}

[rev3r]

It won't be because this would be compiled to:

bool lockWasTaken = false;
var temp = obj;
try
{
    Monitor.Enter(temp, ref lockWasTaken);
    //code that needs to be synchronized
}
catch (Exception e)
{
    //rolling back changes in case of error
}
finally
{
    if (lockWasTaken)
        Monitor.Exit(temp);
}

catch would not be after lock block, but rather between try and finally.

[The-Futurist]

@rev3r - A design that tolerates disruptive (to the thread safe updates that are being attempted) exceptions should be regarded as poor design IMHO.

Unless the exception was caused by another thread (which itself should be impossible when holding the lock, unless its like ThreadAbortException) it will be caused by data accessed while holding the lock. That data is by definition shared and so would imply that every thread that tries to do the updates would encounter the exception.

This means we'd get never ending lock-update-exception-rollback-unlock cycles over and over, never recovering, whenever any thread enters the lock.

You could argue that an input argument might lead to an exception but in that case you'd validate arguments and throw before attempting to acquire the lock.

Please suggest some types of exceptions you'd consider acceptable in the pattern you're suggesting.

[rev3r]

@Korporal what if the exception was only one-time, and thrown in the middle of the lock block? You would release resources that are broken.

That cycle would be only if lock can't be finished properly, and executes one time for each thread. So that is good, because it would protect correct resource's state.

[The-Futurist]

@Korporal what if the exception was only one-time, and thrown in the middle of the lock block? You would release resources that are broken.

That cycle would be only if lock can't be finished properly, and one time for each thread. So that is good, because it would protect correct resource's state.

@rev3r

I'm arguing that such exception would likely not be one time. They would be caused by the data being accessed and so no thread could enter the code and avoid the exception once some other thread has encountered the exception and rolled back.

By rolling back you set the state to its pre-exception state and thus garuantee that the next locking thread will hit the same exception.

What kinds of exceptions might you get? Can you suggest an exception not caused by accessing shared state?

[rev3r]

@Korporal Even if exception is not one time, the half-mutated resource problem will still be an issue. That's fixed with catch.

Imagine you need to change few fields in DB. Without rollback, exception would release the lock half-way through. Now each thread is free to read/write wrong state.

And it's not only DB, everytime you need multiple steps in one lock you could encounter this issue.

Even C# language designer (Eric Lippert) and C# async concept designer (Jeffrey Richter) argues that it's a real issue with lock block.

[The-Futurist]

@Korporal Even if exception is not one time, the half-mutated resource problem will still be an issue. That's fixed with catch.

Imagine you need to change few fields in DB. Without rollback, exception would release the lock half-way through. Now each thread is free to read/write wrong state.

And it's not only DB, everytime you need multiple steps in one lock you could encounter this issue.

Even C# language designer (Eric Lippert) and C# async concept designer (Jeffrey Richter) argues that it's a real issue.

@rev3r - But performing IO (e.g. Database) while holding a lock is itself regarded as poor design and so would be eliminated during a code review.

By accessing a state S in such a way we generate an exception wholly dependent on S then by resetting the state back to S ("rolling back") we guarantee that the next thread to attempt this will generate the same exception, reset it and so on...

In Windows for example (the rentrant kernel) if kernel code encounters such situations it "crashes" the system, that is to say it calls KeBugCheck() it does not attempt to rollback.

Rolling back is no good because the problem was already present before the thread began its updates.

What needs to be rolled back is the updates that caused the corruption not the updates made by code that later encounters the corruption.

[rev3r]

@Korporal Even in-memory data structure manipulation can lead to exception.

In my opinion better than releasing the lock prematurely is to allow crashing. So lock, which guarantees release of the lock, is not a great solution at all (without catch).

[The-Futurist]

@Korporal Even in-memory data structure manipulation can lead to exception.

In my opinion better than releasing the lock prematurely is to allow crashing. So lock, which guarantees release of the lock, is not a great solution at all (without catch).

@rev3r Yes, crashing might be best but in this case a lock block with a wholly contained try/catch is the ideal pattern, I see no advantage here in your proposed language change.

[rev3r]

@Korporal It's the same as try-catch inside lock but without ugly nesting and (effectively) double try.

This means cleaner and little bit faster code.

[yaakov-h]

It only removes a single keyword.

@HaloFour's alternative seems to achieve the result you're after, with no language changes required.

At the IL level you still have a double-try, but I'd expect that to remain so that we only catch exceptions from the block of user code, not from Monitor.Enter itself.

Relevant Sharplab playground.

[The-Futurist]

@rev3r - Well these are just my opinions, but encouraging designs that suggest to developers their code can recover or rollback is a bad idea. It's bad because I don't really think you can rollback the corruption because this was likely caused by an earlier thread doing updates not the thread that encounters an exception because of those updates and it's bad because it increases the prospect of a false sense of security.

What if this lock was nested? I mean what if you're thread gets a lock in method Outer, then calls method Inner and gets another lock, then hits the exception and rolls back those Inner updates? Should we not also rollback any updates made in Outer?

[rev3r]

@yaakov-h And nesting. We care about this, so we removed extra nesting in using block and use await instead of callbacks.

@Korporal Exception in Inner will be caught by Inner catch and rollback only Inner changes.

[The-Futurist]

@yaakov-h And nesting. We care about this, so we removed extra nesting in using block and use await instead of callbacks.

@Korporal Exception in Inner will be caught by Inner catch and release only Inner changes.

@rev3r - So the code in Outer has no idea that a critical set of updates expected by calling Inner were in fact not performed? What if the code in Outer assumes, relies on Inner having made those updates?

Locking code is often highly non intuitive and requires a great deal of care and attention, even with very strict, simple policies we can get almost unfathomable bugs, the "rolling back" idea makes this worse. I read Lippert's article and I don't see him actually advocating any rollback.

[rev3r]

@Korporal We could rethrow if we want.

[The-Futurist]

@Korporal We could rethrow if we want.

@rev3r - But that simply won't do, how does the called method know it should rethrow? Should it simply rethrow or throw a new kind of LockRollbackException? Should Outer then only itself rollback if it sees that exception?

Very quickly this gets unfathomable IMHO and rolling back updates made with a lock held is fundamentally bad design. It cannot help or improve system quality and I've yet to see an example that makes me reconsider this.

[rev3r]

@Korporal The same questions are asked when handling normal exceptions.

I meant normal rethrow, not some special one.

In response to blog and rollback:

But an even better reason is because small, simple lock bodies minimize the chance that the thing in there is going to throw an exception. It's also easier to rewrite mutating lock bodies to have rollback behaviour if they don't do very much to begin with.

He enumerates 3 solutions, each with issues.

[The-Futurist]

I need to see an example, even Mr. Lippert didn't include one.

[The-Futurist]

In SQL/CLR user's managed code cannot take locks, code must be static. There's reason this is disallowed.

[rev3r]

@Korporal I can't provide real world examples, because I avoid lock and try to use other things instead (Volatile, Interlocked, WaitAsync).

[The-Futurist]

@rev3r - So how would you handle rollback if you were using spinlocks/InterlockCompreExchange? Can you think of an example with that where rollback helps?

[CyrusNajmabadi]

This means cleaner ... code.

We want make code cleaner for very common scenarios. Extremely niche scenarios taht are already served by existing constructs don't need time spent making them just a tiny bit nicer.

[The-Futurist]

@rev3r - The reason I've argued as I have so far is that I want to understand the scenarios that can cause a code lock block to encounter an exception. If we cannot do this then fine, but if we can we should.

These are the scenarios that spring to mind, there are probably more:

1. An argument passed to the method was not validated.
2. ThreadAbortException.
3. Timeout Exception.
4. CLR, execution exception.
5. The data being accessed and manipulated in the code block is corrupted.

In the first four cases rollback is not possible, in case 1. it is not really necessary, 2. Is caused by some other thread, 3. is caused by abuse of the lock concept itself and 4 is either a system bug, OS bug or hardware problem.

This leaves 5.

Now in case 5. The exception is not the result of actions performed after our thread acquired the lock but by actions performed by a thread before we acquired the lock. Since our thread did not perform those actions it cannot undo them.

[rev3r]

@Korporal I can't provide good example either, where it's critical, but I'm not that experienced developer myself.

I just think there as some scenarios but I could be wrong and this feature would be never used in practice.

Either way thanks for constructive discussion 👍 I'll leave this open, maybe someone smarter could come up with some good examples.

[The-Futurist]

@rev3r - You're very welcome and do not think I was trying to press you or anything, I have worked on systems that rely on locks several times and the subject is very interesting and so easy to mess up, I've certainly messed things up several times myself!

[mattwar]

Rolling back the state is often a complicated proposition. That's what transactional memory tries to solve. But it also ends up having the same problem that @Korporal points out.