RFR: JDK-8241883: (zipfs) SeekableByteChannel:close followed by SeekableByteChannel:close will throw an NPE

[Langer, Christoph]

Hey Lance,

I believe the call to

if(!isOpen())
    return;

should be placed before pulling the writeLock, that is, before beginWrite(). Then not every close call necessarily needs to synchronize. Please also note that there should be a space between “if” and “(“. ��

Furthermore, we should build on the already existing locking facilities of super class jdk.nio.zipfs.ByteArrayChannel. In your patch, please remove the new rwlock Object and the new methods beginWrite() and endWrite(). Make their implementations in ByteArrayChannel package private. To make it completely safe that no lock of the whole zipfs is pulled, prefix the new calls to beginWrite() and endWrite() with “super.”. Otherwise there’s a danger that ZIPFS’ beginWrite and endWrite methods are called inadvertently here when no superclass implements them.

Best regards
Christoph

From: nio-dev <nio-dev-bounces at openjdk.java.net> On Behalf Of Lance Andersen
Sent: Mittwoch, 8. April 2020 22:41
To: Alan Bateman <Alan.Bateman at oracle.com>
Cc: nio-dev <nio-dev at openjdk.java.net>
Subject: Re: RFR: JDK-8241883: (zipfs) SeekableByteChannel:close followed by SeekableByteChannel:close will throw an NPE




On Apr 8, 2020, at 4:26 PM, Alan Bateman <Alan.Bateman at oracle.com<mailto:Alan.Bateman at oracle.com>> wrote:


On 08/04/2020 20:36, Lance Andersen wrote:
:
——————————
$ hg diff
diff -r f4c174bf0276 src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java
--- a/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java      Tue Apr 07 09:03:05 2020 -0400
+++ b/src/jdk.zipfs/share/classes/jdk/nio/zipfs/ZipFileSystem.java      Wed Apr 08 11:07:30 2020 -0400
@@ -876,6 +876,7 @@
     // channel is closed
     private class EntryOutputChannel extends ByteArrayChannel {
         final Entry e;
+        final ReadWriteLock rwlock = new ReentrantReadWriteLock();

         EntryOutputChannel(Entry e) {
             super(e.size > 0? (int)e.size : 8192, false);
@@ -892,11 +893,24 @@

         @Override
         public void close() throws IOException {
-            // will update the entry
-            try (OutputStream os = getOutputStream(e)) {
-                os.write(toByteArray());
+            beginWrite();
+            try {
+                if(!isOpen())
+                    return;
+                // will update the entry
+                try (OutputStream os = getOutputStream(e)) {
+                    os.write(toByteArray());
+                }
+                super.close();
+            } finally {
+                endWrite();
             }
-            super.close();
+        }
+        private final void beginWrite() {
+            rwlock.writeLock().lock();
+        }
+        private final void endWrite() {
+            rwlock.writeLock().unlock();
         }

That is the equivalent of close synchronizing on any object. Are there any code paths use the readLock?

In the case of the close method above, ByteArrayChannel::toByteArray uses a readLock  ByteArrayChannel::close uses a writeLock

Best
Lance


-Alan

[cid:image001.gif at 01D60E42.446B2030]<http://oracle.com/us/design/oracle-email-sig-198324.gif>

<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
Lance.Andersen at oracle.com<mailto:Lance.Andersen at oracle.com>




-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200409/8d1d07f9/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: image001.gif
Type: image/gif
Size: 658 bytes
Desc: image001.gif
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200409/8d1d07f9/image001-0001.gif>