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.

Print | posted @ Tuesday, August 28, 2007 2:00 AM

Comments on this entry:

Gravatar # re: What’s Wrong With This Code (#17)
by skware at 8/28/2007 2:45 AM

Read backwards.
()sserpmoC ni maertSpiz eht esolc ot togrof uoY
()esolC.maertSpiz
  
Gravatar # re: What’s Wrong With This Code (#17)
by scott at 8/28/2007 2:56 AM

He shoots! He scores!

Winner!
  
Gravatar # re: What’s Wrong With This Code (#17)
by Scott McMaster at 8/28/2007 3:07 AM

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).
  
Gravatar # re: What’s Wrong With This Code (#17)
by Jia Li at 8/28/2007 3:52 AM

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
  
Gravatar # re: What’s Wrong With This Code (#17)
by darin at 8/28/2007 10:26 AM

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.
  
Gravatar # re: What’s Wrong With This Code (#17)
by Chris at 8/28/2007 11:26 AM

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)
  
Gravatar # re: What’s Wrong With This Code (#17)
by scott at 8/28/2007 12:32 PM

@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.
  
Gravatar # why no using?
by Josh at 8/28/2007 6:29 PM

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
  
Gravatar # re: What’s Wrong With This Code (#17)
by scott at 8/29/2007 1:44 AM

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.
  
Gravatar # re: What’s Wrong With This Code (#17)
by Petar shomov at 8/29/2007 9:07 AM

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.).
  
Gravatar # re: What’s Wrong With This Code (#17)
by Lionel at 8/29/2007 10:01 AM

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

  
Gravatar # re: What’s Wrong With This Code (#17)
by Alois Kraus at 8/29/2007 2:24 PM

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
  
Gravatar # re: What’s Wrong With This Code (#17)
by Duy at 8/29/2007 2:33 PM

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.
  
Gravatar # re: What’s Wrong With This Code (#17)
by scott at 8/29/2007 6:03 PM

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

  
Gravatar # re: What’s Wrong With This Code (#17)
by Lionel at 8/30/2007 12:32 PM

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
  
Gravatar # re: What’s Wrong With This Code (#17)
by luyong at 9/3/2007 2:47 PM

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

Your comment:

Title:
Name:
Email:
Website:
 
Italic Underline Blockquote Hyperlink
 
 
Please add 8 and 2 and type the answer here: