return statement before finally

Question

I have a c# program which acts as client and many client program which is again a c# windows application connects to this c# server program to read data from sqlite database. To avoid lock issues when connecting multiple clients i have used below code,

System.Threading.Monitor.Enter(Lock);

try                       
{                     
    filter.Execute();//get data from database
    Request.Clear();
    return filter.XML;//create xml and return to client
}
finally
{
    System.Threading.Monitor.Exit(Lock);
}

server gets hangs some times and need to restart the server program. Is it a good practce to make the return statement before finally?

Regards sangeetha

This is just the hard way to write the **lock** statement. It is otherwise useless to ensure that only one process can use SqlLite, you will need to use a named Mutex instead. — Hans Passant, May 11 '13 at 11:07

bash.d · Accepted Answer · 2013-05-11T11:02:58.233

From MSDN

By using a finally block, you can clean up any resources that are allocated in a try block, and you can run code even if an exception occurs in the try block. Typically, the statements of a finally block run when control leaves a try statement. The transfer of control can occur as a result of normal execution, of execution of a break, continue, goto, or return statement, or of propagation of an exception out of the try statement.

Within a handled exception, the associated finally block is guaranteed to be run. However, if the exception is unhandled, execution of the finally block is dependent on how the exception unwind operation is triggered. That, in turn, is dependent on how your computer is set up. For more information, see Unhandled Exception Processing in the CLR.

VladL · Answer 2 · 2013-05-11T16:42:32.053

2

Yes, it's what finally statement is for. It will be executed after the return, even if an exception occurs

EDIT: this simple code will show you, that catch block is not needed for finally block to be executed

public Form1()
{
    InitializeComponent();
    check();
}

private string check()
{
    try
    {
        return String.Empty;
    }
    finally
    {
        MessageBox.Show("finally");
    }
}

edited May 11 '13 at 16:42

answered May 11 '13 at 10:25

VladL

12,769
10
63
83

@bash.d it's defined behavior. Have you read what you've posted? :D "Typically, the statements of a finally block run when control leaves a try statement. The transfer of control can occur as a result of normal execution... or return statement" – VladL May 11 '13 at 10:51
OK... You'll never stop learning, although this is very strange. – bash.d May 11 '13 at 11:03
@VladL. I don't think finally will execute with the code as it is - there is no exception handling using a catch clause. Unhandled exceptions change everything for finally. – Andy Brown May 11 '13 at 11:28
1

@AndyBrown why think too long, create the simple project and test my edit ;) – VladL May 11 '13 at 16:43
1

@VladL. Fair enough, my use of "I don't think" was too vague: "I know". The [C# language documentation](http://msdn.microsoft.com/en-us/library/vstudio/zwc8s4fz.aspx) _states_ that finally will not always get called (in the case that the exception is unhandled). While you have shown a case where it does get called, that doesn't show that it can't still remain uncalled in other scenarios. – Andy Brown May 11 '13 at 18:49

Andy Brown · Answer 3 · 2013-05-11T20:48:48.013

As you have no catch block, there is no guarantee that finally will be executed. From MSDN - try-finally (C# Reference) and "Locks and exceptions do not mix" (Eric Lippert)

Within a handled exception, the associated finally block is guaranteed to be run. However, if the exception is unhandled, execution of the finally block is dependent on how the exception unwind operation is triggered. That, in turn, is dependent on how your computer is set up.

And from the link that is then mentioned (Unhandled Exception Processing In The CLR) there are then various considerations which could mean you end up with a terminated thread. I don't honestly know if that would leave you with a lock on the lock object though.

If you wanted to ensure that:

you release the lock; but
you don't want to handle the exception at this level, instead you want it handled by a higher level exception handler

Then do:

TheXmlType xml = null;
Monitor.Enter(Lock);
bool inLock = true;
try {
  ...
  xml = filter.Xml; // put this here in case it throws an exception
  inLock = false; // set this here in case monitor.exit is to 
    // throw an exception so we don't do the same all over again in the catch block
  Monitor.Exit(Lock);
  return xml; // this is fine here, but i would normally put it outside my try
}
catch (Exception) {
  if (inLock) Monitor.Exit(Lock);
  throw;
}

But, please note: don't use catch (Exception) to hide exceptions, this is only ok if you are re-throwing the exception. People also recommend you have a single return statement, and usually this would be outside your try block.

EDIT:

Confirmed with a test program, and from MSDN - Exceptions in Managed Threads

Starting with the .NET Framework version 2.0, the common language runtime allows most unhandled exceptions in threads to proceed naturally. In most cases this means that the unhandled exception causes the application to terminate.

So, if you don't handle your exception, your application will crash (and you won't have to worry about the lock). If you do handle it, then your original code would execute it's finally block and you are ok.

EDIT 2: Test code updated as it did not properly illustrate a non-firing finally:

class Program
{ 
    static void Main(string[] args) {
        Program p =new Program();
        p.Start();
        Console.WriteLine("done, press enter to finish");
        Console.ReadLine();
    }

    private readonly object SyncRoot = new object();
    ManualResetEvent mre = new ManualResetEvent(false);

    private void Start() {
        /*
         * The application will run the thread, which throws an exception
         * While Windows kicks in to deal with it and terminate the app, we still get 
         * a couple of "Failed to lock" messages
         * */

        Thread t1 = new Thread(SetLockAndTerminate);
        t1.Start();
        mre.WaitOne();
        for (int i = 0; i < 10; i++) {
            if (!Monitor.TryEnter(this.SyncRoot, 1000)) {
                Console.WriteLine("Failed to lock");
            }
            else {
                Console.WriteLine("lock succeeded");
                return;
            }
        }
        Console.WriteLine("FINALLY NOT CALLED");
    }
    public int CauseAnOverflow(int i)
    {
        return CauseAnOverflow(i + 1);
    }
    public void SetLockAndTerminate() {
        Monitor.Enter(this.SyncRoot);
        Console.WriteLine("Entered");
        try {
            mre.Set();
            CauseAnOverflow(1); // Cause a stack overflow, prevents finally firing
        }
        finally {
            Console.WriteLine("Exiting");
            Monitor.Exit(this.SyncRoot);
        }
    }
}

score 0 · Answer 4 · edited May 23 '17 at 12:13

Is it bad practice to return from within a try catch finally block?
It is the right way of writing Exception Handling in c# and always finally block will be executed it does not depend on the place of return. I do not know about your code but you should find your problem somewhere else (for example if your code was hosted in IIS I would suspect the state of Lock object in different loaded domains or maybe the lock is just for one incoming call and it happens in database or what is Request.Clear() you don't have a lock block there?). You can easily log the state of incoming calls and find the problem.

return statement before finally

4 Answers4