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
