Hello Peter,
On 13/07/19 2:50 PM, Peter Levart wrote:
> 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.
I'm really glad you brought this up. When I added this end() within the
synchronized block of close(), I had the same question in mind and was
wondering if it's something that we should consider. But I never pursued
it enough to actual come up with a detailed analysis like the one you
have done. Thank you for this.
>
>
> 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
> }
>
Good point. Agreed and it does defeat the code that tries to avoid
having end() invoked more than once.
> 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.
Thank you, I think that answers the question I had (in another thread[1]
before this RFR was raised) about the thread safety expectations of
these classes.
>
> 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.
>
Right, so it all boils down to the expectation of the usage of these
classes. I will need inputs from others who are more knowledgeable about
these classes, to proceed further.
Thank you very much for your detailed review and inputs.
[1]
http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-June/061117.html
-Jaikiran
