4206909 - adding Z_SYNC_FLUSH support to deflaters

[David M. Lloyd]

On 09/10/2009 04:51 PM, Xueming Shen wrote:
> David M. Lloyd wrote:
>> On 09/10/2009 01:47 PM, Xueming Shen wrote:
>>> I think we have enough discussion on this topic, it's time to make a 
>>> final decision. Seems like we are all happy on the
>>> changes in Deflater and new DOS.flush(mode), the only difference is 
>>> whether or not we should change the existing
>>> behavior of DOS.flush() should use Z_SYNC_FLUSH by default.
>>
>> The question is, is there really such code out there?  It can't really 
>> have any useful semantics that I can see.  My irrelevant vote is 
>> firmly on "yes" for this.
>>
>> - DML
> -The question is, is there really such code out there?
> 
> I honestly don't know, this is why it is called "compatibility risk", 
> similar to the investment risk in the stock market, you
> lose or gain only after you put the real money in:-) Though my feeling 
> is it is very "likely" this change is going to break
> someone's code given DOS is designed/implemented to be easily used as a 
> "normal" OutputStream.
> 
> -It can't really have any useful semantics that I can see
> 
> DOS abstracts two functionalities, a deflater and an output stream, both 
> the deflater and output stream have their own
> "flush" semantics. A flush on the deflater changes the final output, 
> degrades the compression, basically it changes the
> "function" of the deflater. A flush on the OutputStream however is 
> supposed to push out the "buffered" output (accumulated
> for whatever reason, mostly for the i/o performance), it does not change 
> the final result/output. So arguably it should not be
> a too bad idea to also provide two types of "flush" to flush the 
> deflater and/or to flush the output stream, the existing
> DOS.flush() "accidentally" provides the semantics of the outputstream's 
> flush(), with the help of the newly added flush(mode),
> now the developer have the choice of doing whatever "flush" they want to 
> do.

This is not unusual in the JDK or elsewhere - that a stream calls the 
underlying stream's flush() method I mean - and I think in the (unlikely) 
event that someone wants to flush the underlying stream only, they could 
simply call .flush() directly on that stream.  And furthermore, if they 
want to *prevent* the flush() method from calling the underlying flush(), 
they can simply interpose a FlushIgnoringOutputStream which changes flush() 
to a no-op and passes other calls through.  So I'd say "no" to having a 
separate flush method that does *that*.

That all said, I guess I should qualify my "yes" with the statement that if 
the final decision were ultimately "no", it would be really, really trivial 
for someone to subclass or wrap DOS with a version which does the full sync 
flush instead of the pass-thru that your current webrev has.  And come to 
think of it, if you're going to provide your own deflater (which often 
happens) you need to subclass DOS anyway to avoid leaking Deflaters on 
stream close (since the "usesDefaultDeflater"-based auto-cleanup mechanism 
is defeated in this case).  So maybe it wouldn't be so bad to leave it as is...

- DML