On Fri, Mar 2, 2018 at 12:49 PM, Xueming Shen <xueming.s...@oracle.com> wrote:
> Hi David,
>
> (1) Deflater.deflate(Bytebuffer)
> the api doc regarding "no_flush" appears to be the copy/paste of the
> byte[] version
> without being updated to the corresponding ByteBuffer?
You're right, I missed that one. I've incorporated this fix locally:
--- 8< --- cut here --- 8< ---
diff --git a/src/java.base/share/classes/java/util/zip/Deflater.java
b/src/java.base/share/classes/java/util/zip/Deflater.java
index 524125787a8..9cf735a9477 100644
--- a/src/java.base/share/classes/java/util/zip/Deflater.java
+++ b/src/java.base/share/classes/java/util/zip/Deflater.java
@@ -483,7 +483,7 @@ public class Deflater {
* <p>This method uses {@link #NO_FLUSH} as its compression flush mode.
* An invocation of this method of the form {@code deflater.deflate(b)}
* yields the same result as the invocation of
- * {@code deflater.deflate(b, 0, b.length, Deflater.NO_FLUSH)}.
+ * {@code deflater.deflate(output, Deflater.NO_FLUSH)}.
*
* @param output the buffer for the compressed data
* @return the actual number of bytes of compressed data written to the
--- 8< --- cut here --- 8< ---
I'll post an amended patch once we work out the other points below
(and any other feedback that comes in the meantime).
> (2) Inflater.inflate()
> a "inputConsumed" field is added to handle the "bytes-read" if a DFE is
> being thrown.
>
> While the API doc specifies that "if the setInput(bytebuffer) method
> was call ...."
> it appears the inputPos/bytesRead are being updated for the
> "setInput(byte[]) case
> as well. This might be a desirable change, but is an "incompatible"
> behavior change
> as well. I doubt if there is any existing app depending on this
> existing behavior though,
> it's really hard, if not impossible, to try to recover from this kind
> of error
I agree; this was implemented based on Alan's points but I could go
either way with it.
I had a thought that I could update the position only in the buffer
input case, not the array input case, but this would still have the
effect of changing the behavior of Inflater.getRemaining() when the
input is a buffer, which is still technically an incompatibility. So
the three options are:
* Always update the input position on exception
* Only update the input position on exception when the input is a ByteBuffer
* Never update the input position on exception
AFAICT there are no inherent technical issues with any of these
approaches beyond the obscure compatibility change. Since the
behavior was previously unspecified, I think that decreases the
compatibility risk even further, so I'm inclined to think that #1 is
the best overall choice.
> There might also bring in a little inconsistency, as we are updating
> the "input" in this
> case, but why not the "output" buffer? It's true there is no way to do
> that with the
> inflate(byte[] ...), it's kinda doable for the inflate(ByteBuffer) and
> in fact there might
> be bytes that have been written into the output ByteBuffer in this
> case.
Since there is no way to get the output bytes consumed in the array
case, this is not an incompatible change, so I am happy to include it
in my next revision. However I think it only makes sense to do this
if it is consistent with input; i.e. if option #3 above is chosen then
it would be inconsistent to update the output position on exception
and we should not do it.
> (3) there are usages like
> Math.max(buf.limit() - outputPos, 0);
> just wonder if there is any reason not using existing api method.
> Math.max(buf.remaining(), 0);
This is because the buffer's position can change between the call to
buf.position() and buf.remaining(), which can have the ultimate effect
of allowing invalid memory accesses. By acquiring the position and
limit each one time, I can guarantee some consistent view of these
properties. This is consistent with (for example) sun.nio.ch.IOUtil.
Also just as a general principle, buf.remaining() reads both the limit
and position, and since I already have the position, it makes sense to
just read the limit which is the only other piece of data that must be
read in order to correctly calculate these values.
Thanks for the feedback.
--
- DML
