SslStream-compatible ReadLine extension

Question

This code is a Extension to just be able to read a line quickly and efficiently with support for SslStream.

What i'm looking to achieve:

• Lower CPU usage by improving how it stores bytes for being converted to string later
• Lower CPU usage by possibly not reading per-byte some how. In my debug tests its using up the most CPU by far. 33%/55% of samples are the ReadByte

So your probably asking why im using a Queue as a buffer? Basically the Queue allows me to more efficiently store bytes in an array format. byte[] is un-acceptable due to the fact its size can't be changed dynamically (you would have to make a new byte, move over the old data, add the new, replace the old byte[], in-efficient)

queue however (or Stack if you .Reverse() it with linq) lets you add bytes whenever you want and then .ToArray when you need it as a byte[] Same thing can be done with List<byte> but I dont feel like it's the best tool for the job in this case.

Another thing I could do here is just convert with Encoding.GetString on every byte and StringBuilder it but that seems to use more CPU and a lot more memory.

Any suggestions?

public static string ReadLine(this Stream stream, ref int bodySize, Encoding encoding) {
    bool bodySizeWasSpecified = bodySize > 0;
    byte b = 0;
    Queue<byte> Buffer = new Queue<byte>();
    while (true) {
        #region Try Get 1 Byte from Stream
        try {
            int i = stream.ReadByte();
            if (i == -1) {
                break;//stream ended/closed
            }
            b = (byte)i;
        } catch {
            return null;//timeout //not authenticated context
        }
        #endregion
        #region If there's a body size specified, decrement back 1
        if (bodySizeWasSpecified) {
            bodySize--;
        }
        #endregion
        #region If Byte is \n or \r
        if (b == 10 || b == 13) {
            #region If ByteArray is Empty and the byte is \n reloop so we dont start with a leading \n
            if (Buffer.Count == 0 && b == 10) {
                continue;
            }
            #endregion
            #region We hit a newline, lets finish the reads here.
            break;
            #endregion
        }
        #endregion
        #region Add the read byte to the Byte Array
        Buffer.Enqueue(b);
        #endregion
        #region Break if bodysize was greater than 0 but now its 0
        if (bodySizeWasSpecified && bodySize == 0) {
            break;
        }
        #endregion
    }
    return encoding.GetString(Buffer.ToArray());
}

Answer 1

So your probably asking why im using a Queue as a buffer? Basically the Queue allows me to more efficiently store bytes in an array format. byte[] is un-acceptable due to the fact its size can't be changed dynamically (you would have to make a new byte, move over the old data, add the new, replace the old byte[], in-efficient)

Well if you read the docs for Stream.ReadByte() you would see this

Notes to Inheritors

The default implementation on Stream creates a new single-byte array and then calls Read(Byte[], Int32, Int32). While this is formally correct, it is inefficient. Any stream with an internal buffer should override this method and provide a much more efficient version that reads the buffer directly, avoiding the extra array allocation on every call.

and if we dig into the reference source of Stream we would stumble over the default implementation

public abstract int Read([In, Out] byte[] buffer, int offset, int count);

// Reads one byte from the stream by calling Read(byte[], int, int). 
// Will return an unsigned byte cast to an int or -1 on end of stream.
// This implementation does not perform well because it allocates a new
// byte[] each time you call it, and should be overridden by any 
// subclass that maintains an internal buffer.  Then, it can help perf
// significantly for people who are reading one byte at a time.
public virtual int ReadByte()
{
    Contract.Ensures(Contract.Result<int>() >= -1);
    Contract.Ensures(Contract.Result<int>() < 256);

    byte[] oneByteArray = new byte[1];
    int r = Read(oneByteArray, 0, 1);
    if (r==0)
        return -1;
    return oneByteArray[0];
}  

which isn't overwritten neither by NetworkStream nor by AuthenticatedStream (which is inherited by SslStream).

That beeing said, let's dig into the code...

Region's are considered as an antipattern especially if they occur inside a method. Take a look at Are #regions an antipattern or code smell?

Take e.g

    #region If Byte is \n or \r
    if (b == 10 || b == 13)
    {  

this should be replaced by either

    if (b == '\n' || b == '\r')
    {  

or by

    const char NewLine = '\n';
    const char Return = '\r';

    if (b == NewLine || b == Return)
    {

I don't really like the catch here because it isn't specific. You should consider to remove the try..catch completely and let the calling code handle any thrown exception. This leads to another problem: The method is public but you don't validate if stream is null. You will just let the exception be thrown and catched by the catch I mentioned.

---

I would add two overloaded ReadLine() methods to distinguish between needing the bodySize and let these methods return an IEnumerable<byte> which I would then convert to a byte[]. The methods would be called like so

 public static string ReadLine(this Stream stream, ref int bodySize, Encoding encoding)
{
    if (stream == null) { throw new ArgumentNullException("stream"); }

    byte[] result;

    if (bodySize > 0)
    {
        result = stream.ReadLine(bodySize).ToArray();
        bodySize -= result.Length;
    }
    else
    {
        result = stream.ReadLine().ToArray();
    }

    return encoding.GetString(result);

}

which is much easier to read.

The methods themself could look like so

private const char NewLine = '\n';
private const char Return = '\r';

private static IEnumerable<byte> ReadLine(this Stream stream, int bodySize)
{
    int i;
    int counter = 0;
    while (bodySize > 0 && (i = stream.ReadByte()) > 0)
    {
        byte b = (byte)i;
        if (b == NewLine || b == Return)
        {
            // Break only if it isn't the first byte.
            if (counter > 0 || b != NewLine) { yield break; }

            continue;

        }
        counter++;
        bodySize--;
        yield return b;
    }
}

private static IEnumerable<byte> ReadLine(this Stream stream)
{
    int i;
    int counter = 0;
    while ((i = stream.ReadByte()) > 0)
    {
        byte b = (byte)i;
        if (b == NewLine || b == Return)
        {
            // Break only if it isn't the first byte.
            if (counter > 0 || b != NewLine) { yield break; }

            continue;

        }
        counter++;
        yield return b;
    }
}  

or if we want to be clever we could change the second method like so

private static IEnumerable<byte> ReadLine(this Stream stream)
{
    return stream.ReadLine(int.MaxValue);
}

Please note that the provided code isn't tested.

Answer 2

Design

• First of all, is there a particular reason why you're not using a StreamReader?
• Reading one byte at a time is a bad idea if you care about performance, but if you read multiple bytes at once there's a risk of reading too much, and not all streams support reverting to a previous position. You'll want to read into a buffer for performance sake, but you don't want to throw away what's left in that buffer. That's why StreamReader is a class that wraps a given stream, instead of a static, stateless (extension) method.
• Judging by the name and first argument type this looks like a general-purpose, reusable method (reading lines from any kind of stream), but it also contains a few very specific notes (timeout, not authenticated context). If it's meant to be used only in a specific context then I wouldn't make it a public extension method.
• Your method returns null when the given stream throws an exception, and it returns an empty string when the end of the stream has been reached. Not only is that inconsistent with the similarly named StreamReader.ReadLine method, it also hides problems from the caller by gobbling up exceptions, and it doesn't let the caller differentiate between an actual empty line and the end of the stream. This behavior isn't documented either (which is especially important for general-purpose reusable methods).

---

Performance

• When it comes to performance, always measure! I ran a few tests against code that's using a StreamReader, and the code you've shown here is quite a bit slower. Reading one byte at a time incurs a lot more call overhead, if not anything else, and it shows.
• If you know how much data you need, then it's best to allocate a buffer up-front. However, keep in mind that the number of bytes isn't necessarily equal to the number of characters, depending on encoding and actual string content.
• A Queue or List can be more convenient than an array, but they're still using arrays internally, and as you're adding items they'll occasionally need to allocate a larger array to make room for new items, so they're not necessarily more efficient. Also, several Stream and Encoding methods contain overloads that work with arrays and offset/counts, so you're often better off with arrays.
• Minor point: writing to a ref variable is slightly slower than writing to a local variable. It's not clear to me why bodySize even needs to be passed by ref?

---

Other notes

• Heslacher already pointed to a thread that shows why regions are considered an anti-pattern. You seem to be using them as comments - so why not just use actual comments instead? However, quite a few of them are just repeating what the code already says, so they're just wasting space. In general, I'd only add comments to explain why code does what it does or how it's meant to be used. What it does should usually be clear from the code itself.
• bodySize is not a very descriptive name, and not a name I'd expect in a general-purpose method. I'd rename this to something like maxLength or maxLineLength.
• Edge-case: if bodySize is more than 0, and if the given stream contains more leading '\n' characters than bodySize, then this method ignores bodySize and returns the first full line after those newline characters, with bodySize ending up being negative.

Answer 3

When I test your method with a file stream, it runs forever, because there is no real stop condition. When EOF is reached it just keeps returning an empty string. You should return null or signal otherwise to the client that no more lines was found.

You can maybe do:

  ...
  if (stream.Position >= stream.Length)
    return null;

  return encoding.GetString(Buffer.ToArray());
}

you can't just check the size of the Buffer because a line actually can be empty.

---

Another consideration I would make is the definition of a new line is in the current environment/context. It can be tricky because you maybe receive data from other environments.

.NET provides Environment.NewLine that defines the current environments new line sequence.

If you have a sequence as AAAA\n\n\r\nBBBBB - how many lines is that? I would say 4 ("AAA, "", "", "BBB") but your method gives 3.

---

If you need a ReadLine() method, you probably need to read more than just one line. I would consider to implement a ReadLines() method instead:

public static IEnumerable<string> ReadLines(this Stream stream, Encoding encoding = null)
{
  // TODO: the actual implementation...
  encoding = encoding ?? Encoding.UTF8;
  int bytesRead = 0;
  while ((bytesRead = stream.Read(buffer, 0, bufferSize)) > 0)
  {
    if (new line found)
      yield return encoding.GetString(resultBuffer.ToArray());
    ...
  }
}

You can in this way read larger chunks of bytes from the stream and buffer them in a much more efficient way than when reading one byte at a time.