4206909 - adding Z_SYNC_FLUSH support to deflaters

[Xueming Shen]

Thanks for the comments.

Brandon Long wrote:
> In the new Deflater.deflate, you are checking valid flush values against
> NO_FLUSH twice (and missing FULL_FLUSH)
>   

good catch, a bad copy/paste. More unit tests will be written to catch 
this kind of problem after
we finalize the APIs.

> The new DeflaterOutputStream.flush, you loop as long as some output is
> generated, which can call deflate an extra time.  Normally, code that
> does this loops until there is no def.needsInput and the output is less
> than the size of the output buffer.  I would have thought it would have
> generated extra output, but then the loop would never finish... so I
> guess this must work.
>
>   
The current code works.  But your suggestion of using len < b.length 
(actually it means zlib's
availble_out >  0) seems a better and correct approach, which can same 
an extra round. A
quick scan of the zlib deflate/flush_pending shows it should work, I 
will update to that
after give the code a more careful read and run some tests. Given 
each/every write() loops
till needsInput() returns true, I don't think we need to consider this 
in flush().

> Overall, I'm least happy with #4, since I feel it leaves a bug.  flush()
> on a stream should flush everything I've written to the stream.  This
> bug is that it currently doesn't, and this doesn't fix it.  It makes it
> possible for people to fix it (which isn't possible currently without
> using a completely separate implementation), but it doesn't fix the bug.
>   

Understood and "agreed":-) I'm wiling to change position under more 
pressure:-)  And we
can add that anytime. It's better than put it in now and have to take it 
out later or add in some
ugly workaround.

Sherman


> Brandon
>
> On 09/04/09 Xueming Shen uttered the following other thing:
>   
>> I would like to take the conservative (read compatible) approach for  
>> this issue.
>>
>> (1)expose zlib's flush mode Z_NO_FLUSH, Z_SYNC_FLUSH and Z_FULL_FLUSH
>> (2)add deflate(byte[] b, int off, int len, int flush)
>> (3)document the existing deflate() continues to use Z_NO_FLUSH
>> (4)add DeflaterOutputStream.flush(int flushmode)
>>    and leave DeflaterOutputStream.flush() un-touched.
>>
>> http://cr.openjdk.java.net/~sherman/zipflush/webrev
>>
>> So the sophisticated Deflater/DeflaterOutputStream user can now use all  
>> 3 flush flavors
>> by using the newly added APIs explicitly. "naive" Deflater/DOS user  
>> (means no need for
>> the sync/full_flush) and existing application can continue live with/use  
>> the existing API
>> without any compatibility concern. The only disadvantage of this  
>> approach is the
>> "in-direct flush" use scenario (means the DOS is passed around and the  
>> inherited flush()
>> is invoked by other wrapping classes) will not be benefited from this  
>> change, they have
>> to override the flush() methold before passing the DOS object around,  
>> like what I do
>> in the test case Flush()
>>
>>    static class MyDeflaterOutputStream extends DeflaterOutputStream {
>>        public MyDeflaterOutputStream(OutputStream out) {
>>            super(out);
>> 	}
>>
>>        public void flush() throws IOException {
>>            flush(Deflater.SYNC_FLUSH);
>>        }
>>    }
>>
>> A little inconvenient, but seen like worth the  price of being  
>> compatible, for now. And we
>> still have the choice to do whatever we want to do with this method in  
>> the future should the
>> feedback show I'm wrong and the compatibility does not matter.
>>
>> Opinion? If you guys agree (please also review the webrev) I can start  
>> the "process".
>>
>> Sherman
>>
>>
>>     
>
>