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

[Lance Andersen]

Hi Christoph,

Thank you for the review.

Best
Lance

> On Apr 9, 2020, at 4:20 PM, Langer, Christoph <christoph.langer at sap.com> wrote:
> 
> Hi Lance,
>  
> From: Lance Andersen <lance.andersen at oracle.com <mailto:lance.andersen at oracle.com>> 
> Sent: Donnerstag, 9. April 2020 20:20
> To: Langer, Christoph <christoph.langer at sap.com <mailto:christoph.langer at sap.com>>
> Cc: Alan Bateman <Alan.Bateman at oracle.com <mailto:Alan.Bateman at oracle.com>>; 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
>  
> Hi Christoph,
>  
>  
> Thank you for the feedback as always!
>  
>  
> You’re welcome, as always ��
> 
> 
> On Apr 9, 2020, at 1:50 AM, Langer, Christoph <christoph.langer at sap.com <mailto: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.
>  
> TL;DR: You’re right, keep it!
> Full story: Yes that’s safer. Only checking isOpen() before the lock is not correct. It would probably be a scenario for double checked locking. You could check for isOpen() once outside the lock but you have to as well check for it again after acquiring the lock. Otherwise it can happen that the actual close logic runs more than once. On the other hand, maybe that’s a bit overkill, assuming that a scenario of having multiple threads close an EntryOutputChannel at the same time occurs rather seldom.
> BTW: ByteArrayChannel::close does the “mistake” of only checking for closed before pulling the lock, too. But there closing only means nulling some fields. So if that runs multiple times it’s not an issue.
>  
> 
> 
> Then not every close call necessarily needs to synchronize. Please also note that there should be a space between “if” and “(“. ��
>  
> Done.
> 
>  
> Perfect.
>  
>  
> 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  :-)
>  
> Great.
>  
>  
> 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>
>  
> I’m fine with it.
>  
> Best regards
> Christoph
>  

 <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/c1ad04d1/attachment.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/c1ad04d1/oracle_sig_logo.gif>