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

[Lance Andersen]

Hi Christoph,


Thank you for the feedback as always!


> On Apr 9, 2020, at 1:50 AM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Hey Lance,
>  
> I believe the call to
>  
> if(!isOpen())
>     return;
>  
> should be placed before pulling the writeLock, that is, before beginWrite().

I kept this within the beginWrite() to keep it consistent with how ensureOpen is used.  If the final consensus is to move it, I will.

> Then not every close call necessarily needs to synchronize. Please also note that there should be a space between “if” and “(“. ��

Done.
>  
> 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.

I had planned to do that originally, but for some reason I talked myself out of it but per your suggestion I reversed my original decision  :-)

The webrev can be found at: http://cr.openjdk.java.net/~lancea/8241883/webrev.00/index.html <http://cr.openjdk.java.net/~lancea/8241883/webrev.00/index.html>

Best
Lance
>  
> Best regards
> Christoph
>  
> From: nio-dev <nio-dev-bounces at openjdk.java.net <mailto: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 <mailto:Alan.Bateman at oracle.com>>
> Cc: nio-dev <nio-dev at openjdk.java.net <mailto: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
>  
> <image001.gif> <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>
>  
> 
> 
>  

 <http://oracle.com/us/design/oracle-email-sig-198324.gif>
 <http://oracle.com/us/design/oracle-email-sig-198324.gif> <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/283c737a/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: oracle_sig_logo.gif
Type: image/gif
Size: 658 bytes
Desc: not available
URL: <https://mail.openjdk.java.net/pipermail/nio-dev/attachments/20200409/283c737a/oracle_sig_logo-0001.gif>