Talk about how to write good C# code from a bug

I have noticed one block code yesterday and found a bug in it, note down here to share with you.

Background

=============================================================================

There is a asynchronous operation need to call BeginXXX and EndXXX, a COM object will be returned after BeginXXX called, and most important the COM object must be released manually otherwise it will cause leaking issue.

=============================================================================

Content

A team guy wrote some sample to us, we can refer his code to use. I don’t remember his exact code and use similar code instead.

namespace ConsoleApplication2 { class Program { static void Main(string[] args) { try { FakeReader reader = new FakeReader(); FakeLock fakeLock = reader.BeginRead(); try { reader.EndRead(fakeLock); } finally { fakeLock.Free(); } } catch (Exception ex) { /*handle ex*/} finally { } } } class FakeReader { public FakeLock BeginRead() { return new FakeLock(); } public void EndRead(FakeLock fakeLock) { } } class FakeLock { public static bool freed; ~FakeLock() { if (!freed) throw new ApplicationException("FakeLock shoule be free."); } public void Free() { freed = true; } } }

According the code, you can see he use nested try-finally to make sure fakeLock is freed. Yes, it can work on most parts of time, but what is the truth?

I noticed if there are some code existed between the reader.BeginRead() and try block and unluckily there is happened throw out an exception, the code will stopped immediately and go to outside try-catch-finally. The nested try-finally is skipped and leak issue is occurred.

I sent an email to warn him and his reply is:

he will never add any codes between reader.BeginRead() and try block ; he need the programmer who will refer his code to think clearly the importance before add codes in the special position
It’s obvious he leave the risk to the programmer who will refer his code. This is not reasonable.

BTW even there is no code between reader.BeginRead() and try block, the csc compiler will insert a NOP at the begin of each try. This increase the possibility of code exception. Of course the possibility is very minor.

=============================================================================

Tragedy

I’m a unfortunate person who always watch tragedy. Sadly I saw another related code yesterday afternoon.

static void Main(string[] args) { try { FakeReader reader = new FakeReader(); FakeLock fakeLock = reader.BeginRead(); /* there is a assertion here */ if (fakeLock != null) { fakeLock.Free(); } } catch (Exception ex) { /*handle ex*/} finally { } }

Correct, first it check fakeLock value with null then free it. The logic is perfect right to avoid NullReferenceException. But if the previous assertion is failed, only thespian result is performed.

=============================================================================

Solution

The solution is very simple. Move the fakeLock’s assign into nested try-finaly or upgrade it outside of outside try-catch-finally to global variable. Free it in the last finally block.

static void Main(string[] args) { try { FakeReader reader = new FakeReader(); FakeLock fakeLock = null; try { fakeLock = reader.BeginRead(); reader.EndRead(fakeLock); } finally { fakeLock.Free(); } } catch (Exception ex) { /*handle ex*/} finally { } }

static void Main(string[] args) { FakeLock fakeLock = null; try { FakeReader reader = new FakeReader(); fakeLock = reader.BeginRead(); reader.EndRead(fakeLock); } catch (Exception ex) { /*handle ex*/} finally { if (fakeLock != null) { fakeLock.Free(); } } }

=============================================================================

END.

We should pay more attention to each line of our code and know exactly the running condition of our program.