Ayende @ Rahien

My name is Oren Eini
Founder of Hibernating Rhinos LTD and RavenDB.
You can reach me by phone or email:


+972 52-548-6969

, @ Q c

Posts: 6,124 | Comments: 45,479

filter by tags archive

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?


Tom Quinn


should be

_buffer.Length -1


I think I see a couple issues:

  1. 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.

  2. string.Format (called in two locations) could throw an exception that will not be caught, potentially creating a misleading error.

  3. logger.Log could throw an exception and result in a misleading error.

  4. 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.



No, you don't specify _buffer.Length - 1. You're not indexing the buffer, just specifying its length to the Read() function.

Peter Ibbotson

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"

Peter Ibbotson

BTW Joel since you mentioned out of memory exceptions.

I noticed the other day that the raw Win32 api call WriteFile can throw ERRORNOTENOUGH_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.

John Rayner

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.

Paulo K&#246;ch

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.

Omer Mor

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.

Mario Pareja

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.



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

Comments have been closed on this topic.


  1. RavenDB 3.5 whirl wind tour: I’ll find who is taking my I/O bandwidth and they SHALL pay - one day from now
  2. The design of RavenDB 4.0: Physically segregating collections - about one day from now
  3. RavenDB 3.5 Whirlwind tour: I need to be free to explore my data - 3 days from now
  4. RavenDB 3.5 whirl wind tour: I'll have the 3+1 goodies to go, please - 6 days from now
  5. The design of RavenDB 4.0: Voron has a one track mind - 7 days from now

And 12 more posts are pending...

There are posts all the way to May 30, 2016


  1. RavenDB 3.5 whirl wind tour (14):
    02 May 2016 - You want all the data, you can’t handle all the data
  2. The design of RavenDB 4.0 (13):
    03 May 2016 - Making Lucene reliable
  3. Tasks for the new comer (2):
    15 Apr 2016 - Quartz.NET with RavenDB
  4. Code through the looking glass (5):
    18 Mar 2016 - And a linear search to rule them
  5. Find the bug (8):
    29 Feb 2016 - When you can't rely on your own identity
View all series


Main feed Feed Stats
Comments feed   Comments Feed Stats