trjhtr

I receive messages from a MessageWebSocket connection frequently, which are compressed with zlib. The following code decompresses them to a stream which can be read with JSON.Net's JsonReader.

I'm pretty sure there are some improvements to be done, but can't quite put my finger on what:

private MemoryStream _compressed;
private DeflateStream _decompressor;
private void HandleMessage(object sender, MessageWebSocketMessageReceivedEventArgs e)

 using (var datastr = e.GetDataStream()?.AsStreamForRead())
 using (var ms = new MemoryStream())
 
 datastr.CopyTo(ms);
 ms.Position = 0;
 byte input = new byte[ms.Length];
 ms.Read(input, 0, (int)ms.Length);
 int index = 0;
 int length = input.Length;
 using (var output = new MemoryStream())
 
 if (input[0] == 0x78)
 
 //Remove the zlib header
 _compressed.Write(input, index + 2, length - 2);
 _compressed.SetLength(length - 2);
 
 else
 
 _compressed.Write(input, index, length);
 _compressed.SetLength(length);
 

 _compressed.Position = 0;
 _decompressor.CopyTo(output);
 _compressed.Position = 0;
 output.Position = 0;

 using (var reader = new StreamReader(output))
 using (JsonReader jsreader = new JsonTextReader(reader))
 
 //Deserialize JSON from the jsreader stream
 
 
 

I think you're doing too much in one function, hiding the big picture I'd want to read without going into the implementation details. I'd first start with a broad overview:

using (var reader = new StreamReader(e.GetDataStream().AsStreamForRead())

 SkipOptionalZlibHeader(reader);
 var uncompressedStream = ReadAndDecompressStream(reader);
 DecodeJson(uncompressedStream);

With:

const int ExpectedZLibHeaderMarker = 0x78;
const int ExpectedZLibHeaderSize = 2;

static void SkipOptionalZlibHeader(BinaryReader reader)

 if (!HasZlibHeader())
 return;

 reader.Seek(CalculateZLibHeaderSize(), SeekOrigin.Current);

 bool HasZlibHeader()
 => reader.PeekChar() == ExpectedZLibHeaderMarker;

 long CalculateZLibHeaderSize()
 => return ExpectedZLibHeaderSize;

Stop here for one second. Here you made some HUGE assumptions:

• Compression flags are always 0x78. That's not true because if message contract simply says that it's a compressed zlib stream with zlib header then they may change it without breaking the contract. Also note that there is no reason the raw compressed stream cannot start with that magic number. You may want to try to decode the trimmed stream and if it fails then try to decode it as it had no header. Better: be sure it has (or not) the header and stick to it otherwise you're deploying code that may fail.


• Header size is always two bytes. According to the flags there might be more than that.


• Everything else in the stream is data. No, there is also a footer with a checksum.

Note that we dropped one useless copy because SkipOptionalZlibHeader() simply seek after the header. As exercise for the reader: if underlying stream does not support seeking (check CanSeek property) then you need to read and discard those bytes.

Note that ReadAndDecompressStream() is returning a Stream, the fact that you reuse the same MemoryStream multiple times (very good things if you have a measured and truly beneficial gain in memory footprint and/or performance) is an implementation detail and I'd love to keep its callers unaware of that. Also there is no reason to reuse DeflateStream. We may then implement it as this:

static Stream ReadAndDecompressStream(BinaryReader reader)

 // Go back to beginning for writing
 _output.Position = 0;

 using (var decompressor = new DeflateStream(_output, CompressionMode.Decompress, true))
 
 reader.CopyTo(decompressor);
 

 // Go back to beginning for reading
 _output.Position = 0;

 return _output;

There is something that really bothers me: we're returning an IDisposable object (MemoryStream) but caller does not have its ownership (and then it has not to dispose it). It's not intuitive and it must be documented. Also note that your actual code is working just because an implementation detail: StreamReader calls Dispose() on the provided Stream and your code is working just because disposing MemoryStream has not visible effects (but it's an implementation detail). We also have to do something for that.

Let's first change ReadAndDecompressStream() to return a StreamReader instead and change its name to something more meaningful:

static StreamReader CreateDecompressedReader(BinaryReader reader)

 // Go back to beginning for writing
 _output.Position = 0;

 using (var decompressor = new DeflateStream(_output, CompressionMode.Decompress, true))
 
 reader.CopyTo(decompressor);
 

 // Go back to beginning for reading
 _output.Position = 0;

 return new StreamReader(_output, Encoding.UTF8, false, _output.Length, true);

Now caller has the ownership of the disposable object:

using (var reader = new StreamReader(e.GetDataStream().AsStreamForRead())

 SkipOptionalZlibHeader(reader);
 using (var decompressedReader = CreateDecompressedReader(reader))
 DecodeJson(decompressedReader);

Note that I removed ?., in your original code if e.GetDataStream()? returned null then the first call to datastr.CopyTo() fails. If this may happen then properly handle the case.

As final step you should add some error checking. With I/O things may go wrong then you'd better be prepared (especially if you keep those assumptions in-place), just don't add catch (Exception) and be specific.