What’s Wrong With This Code (#17)

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.

posted on Monday, August 27, 2007 10:00 PM by scott

Comments

Monday, August 27, 2007 7:45 PM by skware

# re: What’s Wrong With This Code (#17)

Read backwards.
()sserpmoC ni maertSpiz eht esolc ot togrof uoY
()esolC.maertSpiz
Monday, August 27, 2007 7:56 PM by scott

# re: What’s Wrong With This Code (#17)

He shoots! He scores!

Winner!
Monday, August 27, 2007 8:07 PM by Scott McMaster

# re: What’s Wrong With This Code (#17)

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).
Monday, August 27, 2007 8:52 PM by Jia Li

# re: What’s Wrong With This Code (#17)

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
Tuesday, August 28, 2007 3:26 AM by darin

# re: What’s Wrong With This Code (#17)

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.
Tuesday, August 28, 2007 4:26 AM by Chris

# re: What’s Wrong With This Code (#17)

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)
Tuesday, August 28, 2007 5:32 AM by scott

# re: What’s Wrong With This Code (#17)

@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.
Tuesday, August 28, 2007 11:29 AM by Josh

# why no using?

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
Tuesday, August 28, 2007 6:44 PM by scott

# re: What’s Wrong With This Code (#17)

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.
Tuesday, August 28, 2007 10:37 PM by Christopher Steen

# Link Listing - August 28, 2007

Link Listing - August 28, 2007
Wednesday, August 29, 2007 2:07 AM by Petar shomov

# re: What’s Wrong With This Code (#17)

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.).
Wednesday, August 29, 2007 3:01 AM by Lionel

# re: What’s Wrong With This Code (#17)

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 *** always points out more flaws than you though you had.

DOH

Lionel

Wednesday, August 29, 2007 6:44 AM by Noticias externas

# What’s Wrong With This Code (#17)

Scott Allen has a quiz for you: We interrupt this LINQ series with an emergency! Well, there is no real
Wednesday, August 29, 2007 7:24 AM by Alois Kraus

# re: What’s Wrong With This Code (#17)

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
Wednesday, August 29, 2007 7:33 AM by Duy

# re: What’s Wrong With This Code (#17)

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.
Wednesday, August 29, 2007 11:03 AM by scott

# re: What’s Wrong With This Code (#17)

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

Thursday, August 30, 2007 5:32 AM by Lionel

# re: What’s Wrong With This Code (#17)

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
Monday, September 03, 2007 7:47 AM by luyong

# re: What’s Wrong With This Code (#17)

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();
}