+972 52-548-6969
How NOT to write network code
time to read 1 min | 173 words
It is surprising how many bombs you have waiting for you at a low enough level. Here is another piece of problematic code from the same codebase:
// Read the name. var _buffer = new byte[_len]; int _numRead; try { _numRead = stream.Read(_buffer, 0, _buffer.Length); } catch (Exception _ex) { logger.Log(string.Format("Conn: read(): {0}", _ex)); throw new SpreadException(string.Format("read(): {0}", _ex)); } // Check for not enough data. if (_numRead != _len) { logger.Log("Conn: Connection closed during connect attempt"); throw new SpreadException("Connection closed during connect attempt"); }
Can you spot the problem?
Comments
_buffer.Length?
should be
_buffer.Length -1
I think I see a couple issues:
Catching a general Exception isn't a good idea, especially when working with buffers and I/O operations. An out of memory exception could occur and then you're trying to allocate more memory either in the logger.Log call or when creating the SpreadException.
string.Format (called in two locations) could throw an exception that will not be caught, potentially creating a misleading error.
logger.Log could throw an exception and result in a misleading error.
The more interesting issue is:
If we assume that _len contains the length of the name to read off the wire, then the test _numRead != _len might create a misleading error. It is possible that reading from the socket won't return all the data available in the first call - for example, it might take two calls to Read(). It would be more appropriate to read the data off the wire in a loop and test whether the accumulated number of bytes read equals the expected length, or 0 (to indicated all data received/connection closed).
Also the exception doesn't make sense either. Firstly, the socket is likely already connected if you're trying to read from it. :) Furthermore, just because you didn't get all your data doesn't mean the connection has been closed. If the connection was aborted, you'd get an exception. If all the data had been read and the connection was closed, stream.Read() would return 0 to indicated a graceful disconnect.
Tom,
No, you don't specify _buffer.Length - 1. You're not indexing the buffer, just specifying its length to the Read() function.
Since this says network code, I'd guess that lots of the time
_len != _numread, and I'd be real unhappy about that exception calling itself "Connection closed"
I'm guessing that Read() method is problematic. I always read in chunks. There are a whole bunch of ReadFully() methods out there to choose from.
I would start here: www.yoda.arachsys.com/csharp/readbinary.html
BTW Joel since you mentioned out of memory exceptions.
I noticed the other day that the raw Win32 api call WriteFile can throw ERROR_NOT_ENOUGH_MEMORY if it has too many async IO calls running. I'm sure if this is because it calls WriteFileEx under the skin and I may have run into this in the wild with a third party library.
_numRead will only ever ==_len if the received data was that size, but that is not always. It should be if if _numRead <= 0, but it also may not have been initialized.
It's a pretty common to see networking code that expects stream reads to be perfectly aligned with stream writes (since that's largely how it will work when developing locally). In reality though, there is no such guarantee. Reading from a stream could return only a single byte - even though megs have been written on the other side. Similarly, multiple writes might be grouped into a single Read.
Developers need either using a StreamReader/StreamWriter which will block until the built-in delimiter is reached (I assume it's delimiter based), or write "smart" message. These messages normally have a header that says how long the message is (this can be tricky because the X bytes (say 4) you use for a header could itself be chunked into multiple reads)).
There's obviously an expectation here that 1 complete message will be read by each call to Read(). In reality it might take several calls to Read() - or, the single call might actually read multiple messages.
Oh, and the byte allocation could lead to pinning.
From the Stream.Read MSDN docs, the return value "can be less than the number of bytes requested if that many bytes are not currently available, or zero (0) if the end of the stream has been reached.". Boom!
I just had to google for the code and came across a company site that seems to belong to the author of the code you show.
I found this statement:
"[company's name] Selected by [rather big military contractor] for the U.S. Army's Future Combat Systems."
What has it been selected for? I hope just as a bad example.
Reading as a C guy, @int _numRead;@ may never initialize. Comparing stuff without initializing is unpredictable.
Besides, the if should be insed the try block. If you failed to read, you don't check how many you read. You just failed, you've read nothing!
@Paulo: If there was a chance _numRead were uninitialized, this wouldn't compile.
The only code path that can leave it uninitialized is the catch block, which has a throw in it so the if would never happen.
A more subtle misuse of exceptions:
when writing:
try {}
catch (Exception ex)
{
//...
throw new SpreadException(...);
}
You should give ex as inner exception to the constructor of SpreadException.
In addition to the several previously mentioned issues, we can't see when the stream is created, but I bet there isn't much in the way of disposing this resource in the event of an error.
It does throw with an innerexception...
if (_numRead != _len)
_numRead will return _len - 1. At least that is what was happening to me in some strangely similar code.
buffer.Length? how to put it but some one has another choice to choose
you don't specify _buffer.Length - 1. You're not indexing the buffer, just specifying its length to the Read() function.
you don't specify buffer.Length and You're not indexing the buffer, just specifying its length to the Read() function.
Steve,
Look closer, it actually doesn't throw with an inner exception specified. The inner exception is passed to the string.Format method and not the SpreadException constructor.
Comment preview