RFR [14] JDK-8225763 Inflater and Deflater should implement AutoCloseable

[Peter Levart]

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