Reading data from network stream

Question

I have a function to request an update from the server. I provide the query and also indicate the expected length of the response.

    public Byte[] GetUpdate(Byte[] query, int expLength)
    {
        var response = new Byte[expLength];

        lock(Client)
        {
            Stream s = Client.GetStream();
            s.Write(query, 0, query.Length);

            var totalBytesRead = 0;
            var numAttempts = 0;

            while(totalBytesRead < expLength && numAttempts < MAX_RETRIES)
            {
                numAttempts++;
                int bytes;
                try
                {
                    bytes = s.Read(response, totalBytesRead, expLength);
                }
                catch (Exception ex)
                {
                    throw new IOException();    
                }
                totalBytesRead += bytes;
            }

            if(totalBytesRead < expLength)
            {
                // should probably throw something here
                throw new IOException();
            }
        }

        return response;
    }

I don't like the way I have written this code esp with two exception throwing. Is there a more elegant way to write the code?

Answer 1

You should never throw an empty exception. Imagine someone uses your code and receives an empty IOException. What happened? How do you debug this?

First, you need a message that explains the situation. Second, you should use this constructor : IOException(String, Exception). This way, the exception you throw will contain the real exception, the one that causes problem. This will help debugging your code.

if(totalBytesRead < expLength)
{
    // should probably throw something here
    throw new IOException();
}

Yeah, you probably should throw something.. meaningful! Once again, add some information in there. Think about the poor soul that would need to debug this later.

I don't think it's a bad thing to throw exceptions in two separate places, but it's a bad thing to throw meaningless exceptions (I hope the message is clear ahah)

Answer 2

Adding my 2cts to this:

lock(Client)

I'm not quite sure it's a good idea to lock on a property. At least the name suggests that this is a property. Any incorrect use of your class can accordingly easily deadlock it.

while (...)
{
    numAttempts++;

This increases the numAttempts regardless of whether data has been returned by the call to Read. I'd consider a call to Read that returned data a success. Either rename MAX_RETRIES to clarify it's not about catching read-failures or change the semantics of incrementation here.

catch (Exception ex)

It's generally considered bad practice to catch the general Exception. Instead the most specific Exception applicable to the context should be caught. In this case, that would be IOException. All other exceptions that are thrown according to msdn should be propagated as-is since they indicate an error only resolvable through adjustments in the code.

Last but not least: Listen to ToppinFrassi's excellent advice.

Answer 3

I don't entirely agree with the accepted answer. That part that bothers me is the suggesion about using this overload IOException(String, Exception) here:

try
{
    bytes = s.Read(response, totalBytesRead, expLength);
}
catch (Exception ex)
{
    throw new IOException();    
}

I find you should not catch any exceptions there (you already don't do it for this case: s.Write(query, 0, query.Length);). Instead you should create another exception that uses this pattern like UpdateException and provide additionl context about what went wrong. So it could look like this:

try
{
    var update = GetUpdate(query, expLength);
    ...
}
catch(Exception innerException)
{
    throw new UpdateException(versionNumber, queryString, innerException);
}

where

class UpdateException : Exception
{
    public UpdateException(string versionNumber, string queryString, Exception innderException)
        : base($"Could not update to version {versionNumber} from {queryString}.", innerException)
    {}
}

or whatever additional information make sense and could help you to debug it.