What’s Wrong With This Code (#17)

Tuesday, August 28, 2007

We interrupt this LINQ series with an emergency!

Well, there is no real emergency, but there hasn’t been a WWWTC for some time, so …

The following program is suppose to compress its own source code into a Program.cs.zip file, then reverse the compression and produce a Program.txt file.

The problem is: Program.txt always shows up as an empty file!

What’s wrong?

using System;
using System.IO;
using System.IO.Compression;

namespace YippyZippy
{
    
class Program
    {
        
static void Main(string[] args)
        {
            Compress(
@"..\..\Program.cs", @"..\..\Program.cs.zip");
            Decompress(
@"..\..\Program.cs.zip", @"..\..\Program.txt");
        }

        
private static void Compress(string inFileName, string outFileName)
        {
            
FileStream inStream = File.Open(inFileName, FileMode.Open);
            
FileStream outStream = File.Open(outFileName, FileMode.Create);
            
GZipStream zipStream = new GZipStream(outStream, CompressionMode.Compress);

            
try
            {
                
byte[] buffer = new byte[inStream.Length];
                inStream.Read(buffer, 0, buffer.Length);
                zipStream.Write(buffer, 0, buffer.Length);
            }

            
finally
            {
                outStream.Close();
                inStream.Close();              
            }
        }

        
private static void Decompress(string inFileName, string outFileName)
        {
            
FileStream input = File.Open(inFileName, FileMode.Open);
            
GZipStream zipStream = new GZipStream(input, CompressionMode.Decompress);
            
FileStream output = File.Open(outFileName, FileMode.Create);

            
try
            {                
                
int data = zipStream.ReadByte();
                
while (data > 0)
                {
                    output.WriteByte((
byte)data);
                    data = zipStream.ReadByte();
                }
            }
            
finally
            {
                output.Close();
                input.Close();
            }
        }
    }
}

Hint: You can fix the program by adding a single line of code.


Comments
skware Tuesday, August 28, 2007
Read backwards.
()sserpmoC ni maertSpiz eht esolc ot togrof uoY
()esolC.maertSpiz
scott Tuesday, August 28, 2007
He shoots! He scores!

Winner!
Scott McMaster Tuesday, August 28, 2007
Probably want to make the Open's and Close's exception-safe, too (but perhaps that was left out because that code would obscure the key point).
Jia Li Tuesday, August 28, 2007
In method Compress, the gzipstream itself should also be closed, because the default constructor will leave the stream open.

so add zipStream.Close before outStream.Close() in Compress() , the code will work
darin Tuesday, August 28, 2007
The zipStream variable is not disposed in the Compress method which causes the resulting zip file to be corrupted.

Need to add zipStream.Close() in the finally section or use "using" construct instead of try/finally which explicitly calls the Dispose method of the underlying streams.
Chris Tuesday, August 28, 2007
Inside of Decompress, shouldn't you be looking for -1 to know your done reading bytes? The docs say ReadByte returns -1 when a the end of the stream.

while (data > -1)
scott Tuesday, August 28, 2007
@Scott: Yes, I had to do a little obscuring to make this challenging :)

@Chris:

Good point - the code could probably change to make it clear. The ReadByte method should return an unsigned byte, meaning any value less than 0 *should* be a failure, so I think the code is safe - just isn't in tune with the documented behavior.
Josh Tuesday, August 28, 2007
Why didn't you just use a using block with each of the streams? Streams always inherit from IDisposable - if you stick to using this pattern it makes it hard to make mistakes like this.

Josh
scott Wednesday, August 29, 2007
Josh:

Just to impress on the reader that closing a GZipStream is really important. :) Even when the zip stream only looks like an intermediary between two physical files - it needs closed to flush out the compressed bytes.
Petar shomov Wednesday, August 29, 2007
Hm, if the text file is in unicode (UTF-16) the very first byte of the text file will be zero and that will terminate that nasty loop immediately and produce an empty output file.
If you look for -1 as Chris suggested I think you will have no trouble with any type of file(unicode/ansi/binary/etc.).
Lionel Wednesday, August 29, 2007
Well you certainly can't fix this program by adding one line of code. I can see at least three things wrong with it!

1) inStream.Read(buffer, 0, buffer.Length);
This does not do what you think it does. It doesn't read in buffer.Length number of bytes into the buffer. What it actually does is read in a number between 0 and buffer.Length bytes into the buffer and then returns you the number of bytes it has read in. You should use a binary reader if you want to get a byte array of all of the data!

2) You don't Flush or dispose the GZip Stream (I am guessing this is the error that we were ment to be looking for)

3) while (data > 0)
This should be "while (data >= 0)" zipStream.ReadByte() can quite happily return a zero as that is a perfectally valid value for a byte to be!

btw a fun blog post but I guess that is the danger with things like this is that some arogant smart arse always points out more flaws than you though you had.

DOH

Lionel

Alois Kraus Wednesday, August 29, 2007
I think you did close the file stream too early.

FileStream inStream ...
FileStream outStream ...
GZipStream zipStream = new GZipStream (outStream

later you do
outStream.Close();
inStream.Close();

But since the output is stored still in the buffers of zipStream which has not been flushed you will end up with an empty file if no flush of zipStream did occurs. The internal buffer size is as far as I remember 4 KB which means you will be missing the last 0-4KB of every file.
To fix this do simply change it to
zipStream.Close()
outStream.Close();
inStream.Close();

The exception handling is still a bit whacky since you do not aquire the streams inside the finally blocks and close them (if the open was successful) in the finally block if the reference was not null.


Yours,
Alois Kraus
Duy Wednesday, August 29, 2007
As many others got it right: In the Compress() add zipStream.Close() before outStream.Close()
I just want to add that zipStream.Dispose() will also work.
scott Wednesday, August 29, 2007
@Petar: Good point, I didn't actually look at the code when Chris brought this up earlier, but it is wrong to check for > 0.

@Lionel: I don't need a binary reader to get an array of bytes, but one does have to be careful assuming the reader can grab all bytes at once.

Lionel Thursday, August 30, 2007
Opps didn't mean that you needed a binary reader I meant that it was a quick and easy way to get a byte array rather than using a while loop to fill the buffer using the read method.

Lionel
luyong Monday, September 3, 2007
how about this:
try
{
int data = zipStream.ReadByte();
while (true)
{
if (data > -1)
{
output.WriteByte((byte)data);
}
data = zipStream.ReadByte();
if (data == -1)
{
break;
}
}
}
finally
{
output.Close();
input.Close();
}
Comments are now closed.
by K. Scott Allen K.Scott Allen
My Pluralsight Courses
The Podcast!