Hi Jaikiran,
On 7/12/19 9:07 AM, Jaikiran Pai wrote:
The new updated webrev is at
http://cr.openjdk.java.net/~jpai/webrev/8225763/4/webrev/
I don't know if there are any custom subclasses of Inflater/Deflater
around and what they do, but hypothetically consider a subclass of
Deflater similar to this:
public class MyDeflater extends Deflater {
private final Object lock = new Object();
@Override
public void finish() {
synchronized (lock) {
super.finish();
// ... my code ...
}
}
@Override
public int deflate(byte[] output, int off, int len, int flush) {
synchronized (lock) {
// ... my code ...
int result = super.deflate(output, off, len, flush);
// ... my code ...
return result;
}
}
@Override
public int deflate(ByteBuffer output, int flush) {
synchronized (lock) {
// ... my code ...
int result = super.deflate(output, flush);
// ... my code ...
return result;
}
}
@Override
public void reset() {
synchronized (lock) {
super.reset();
// ... my code ...
}
}
@Override
public void end() {
synchronized (lock) {
super.end();
// ... my code ...
}
}
}
Currently, there's no problem with this subclass as Deflater never calls
no overridable method while holding internal lock (zsref). The newly
added close() method is different. It calls end() while holding a lock
(zsref). Suppose that close() is called from one thread, while at the
same time some other overridden method is called from another thread.
Deadlock may happen.
In your patch you are trying hard to prevent calling end() from close()
if end() has already been called (directly or indirectly through
close()) because a hypothetical subclass of Deflater that overrides
end() might not be prepared for end() to be called multiple times (i.e.
there was no specification requiring end() to be idempotent). But even
in your implementation, there is a concurrent race possible that may
allow the overridden end() method to be entered twice. Consider the
following scenario:
Thread A: calls end() explicitly
Thread B: calls close()
There's nothing preventing Thread B from entering overridden part of
end() while thread A is executing this same part of code. Threads
synchronize only at the point where the overridden end() calls super.end():
public class MyDeflater extends Deflater {
@Override
public void end() {
// ... this part of code may execute concurrently
super.end();
// ... this part of code may execute concurrently
}
Well, the subclass may have it's own synchronization in place if it is
used in a concurrent scenario, like in above hypothetical example:
@Override
public void end() {
synchronized (lock) {
// ... this part of code may execute multiple times
super.end();
// ... this part of code may execute multiple times
}
}
But such synchronization has the already mentioned dangerous property of
causing deadlock when close() is called concurrently with end(). In
addition it doesn't prevent parts of overridden end() method to be
executed multiple times.
The following implementation of close() is in no way less effective in
preventing multiple executions of parts of code in overridden end() method:
public void close() {
synchronized (zsRef) {
// check if already closed
if (zsRef.address() == 0) {
return;
}
}
// call end() out of synchronized block to prevent
// deadlock situations in concurrent scenarios
end();
}
...but it is deadlock safe.
It can be argued that synchronization in Deflater is not meant to allow
Deflater to be used from multiple threads effectively, because its API
is not multithread friendly (there are races possible that would render
such usage inpractical). I think synchronization in Deflater is only
meant to keep Deflater internal state consistent which includes state
used in native code. This is important to ensure stable operation of VM
as inconsistency of state in native code may crash the VM. There could
be intentional exploits in untrusted code that would try to crash the
VM. So this is a VM stability measure, not an API that allows effective
multithreaded usage.
In single-threaded usage (that perhaps is the only practical usage),
there are not problems with your implementation of close() as are no
problems with proposed variant above.
Regards, Peter
