2

I have a standalone handler (.ASHX) written in C# that deals with incoming push notifications that carry XML payloads, it then parses these XMLs and extracts and deals with any necessary information taken from them. The issue is I cannot test this locally as our development environment is firewalled too heavily and I'm not allowed to allow exceptions for incoming notifications.

I have tested this on our test environment and the notifications are being received and processed correctly, however after a random code review it looks like there may be an issue with how I am handling responseStreams and streamReaders.

My question is, is this a valid way to handle the closing of these resources or would this potentially cause a NullReferenceException?

What is the standard practice for dealing with these resources, should I be using the using statement to keep them only in use for that scope or will this code suffice?

Stream responseStream = null;
StreamReader reader = null;
string serverResponse = null;

try
{
    responseStream = response.GetResponseStream();
    reader = new StreamReader(responseStream);
    serverResponse = reader.ReadToEnd();
}

finally
{
    if (reader == null)
    {
        reader.Close();
    }

    if (responseStream == null)
    {
        responseStream.Close();
    }

    response.Close();
 }
James Wiseman
  • 29,946
  • 17
  • 95
  • 158
Ben Maxfield
  • 635
  • 7
  • 21

2 Answers2

2

It's better to use using block:

string serverResponse = null;
using (Stream responseStream = response.GetResponseStream())
{
   using (StreamReader reader = new StreamReader(responseStream))
   {
       serverResponse = reader.ReadToEnd();
   }
}

This way all streams will be closed correctly even if there is an exception inside using statement.

Alex
  • 1,657
  • 1
  • 12
  • 6
  • 1
    Nested using statements are nasty imho, they may close streams 'correctly', but exceptions can get lost! http://stackoverflow.com/questions/19238521/handling-exceptions-thrown-by-dispose-while-unwinding-nested-using-statement – Paul Zahra Oct 10 '13 at 08:29
1

I would be tempted to use the following in addition to your code:

responseStream.Seek(0, SeekOrigin.Begin);

using (StreamReader sr = new StreamReader(responseStream))
{
    message = sr.ReadToEnd();
}

and I'd put in a 'catch', what if your responsestream fails in some fashion, e.g. it's closed early at the other end, there's a mismatch in protocols etc etc?

Have a look at the CommunicationException Class

P.S. Personally I don't like nested using statements, although if you must use them then look into AggregateExceptions.

Paul Zahra
  • 9,522
  • 8
  • 54
  • 76
  • Awesome, thanks for your input, I'll have a look at them both, marked first as answer for simple use and implementation but you're answer is also great, thanks – Ben Maxfield Oct 09 '13 at 09:08