Mailing-List: contact notifications-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: jira@apache.org
Date: Wed, 14 Nov 2012 02:26:11 +0000 (UTC)
From: "Tim Halloran (JIRA)" <jira@apache.org>
To: notifications@accumulo.apache.org
Message-ID: <1517824852.111724.1352859972222.JavaMail.jiratomcat@arcas>
In-Reply-To: <1172970709.110565.1352844972253.JavaMail.jiratomcat@arcas>
Subject: [jira] [Commented] (ACCUMULO-853) Fields and parameters that are
 used as locks change to be final (where possible)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit


    [ https://issues.apache.org/jira/browse/ACCUMULO-853?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13496768#comment-13496768 ] 

Tim Halloran commented on ACCUMULO-853:
---------------------------------------

John,

The current code is below:

{code:java}
  public static class CachedBlockRead extends BlockRead {
    private SeekableByteArrayInputStream seekableInput;
    private CacheEntry cb;
    
    public CachedBlockRead(CacheEntry cb, byte buf[]) {
      this(new SeekableByteArrayInputStream(buf), buf.length);
      this.cb = cb;
    }
    
    private CachedBlockRead(SeekableByteArrayInputStream seekableInput, long size) {
      super(seekableInput, size);
      this.seekableInput = seekableInput;
    }
{code}

The changed portion is

{code:java}
  public static class CachedBlockRead extends BlockRead {
    private SeekableByteArrayInputStream seekableInput;
    private final CacheEntry cb;
    
    public CachedBlockRead(CacheEntry cb, byte buf[]) {
      this(new SeekableByteArrayInputStream(buf), buf.length, cb);
    }
    
    private CachedBlockRead(SeekableByteArrayInputStream seekableInput, long size, CacheEntry cb) {
        super(seekableInput, size);
        this.seekableInput = seekableInput;
        this.cb = cb;
      }
{code}

It is not possible to change the *cb* field to be final with the existing constructor scheme. This is because if the private constructor (or the field declaration) assign *null*, the public constructor is not allowed to modify the field. The solution I submitted in the patch is to add this *CacheEntry* object as a parameter to the private constructor and make the assignment there. Alternatively, the private constructor could be refactored into the public one, it is not called except by the public constructor. This may be a better change for understandability of the class, not sure. The class does not seem to work if the *cb* field is null (getIndex would NPE at its second line -- the attempt to synchronize on a null reference), so the change appeared at first blush, pretty reasonable.

The lock in this class appears to protect the state of the *CacheEntry* object referenced by *cb*. Also, the *SeekableByteArrayInputStream* must be thread safe. If this whole class is thread confined, the lock could be removed. I haven't checked this out too deeply at runtime yet.

Technically, this change forces the JVM to insert a memory barrier at the end of object construction (due to the final field). Thus if this object is published by a race condition all its fields will be visible to the thread (currently the other thread could get all nulls regardless of what is passed to the constructor).
                
> Fields and parameters that are used as locks change to be final (where possible)
> --------------------------------------------------------------------------------
>
>                 Key: ACCUMULO-853
>                 URL: https://issues.apache.org/jira/browse/ACCUMULO-853
>             Project: Accumulo
>          Issue Type: Improvement
>            Reporter: Tim Halloran
>            Priority: Minor
>              Labels: patch
>         Attachments: accumulo.patch
>
>
> Examining locking in the code using SureLogic JSure I noticed many locks are not declared final that could be.
> One reference why this may be "bad" is CERT:
> https://www.securecoding.cert.org/confluence/display/java/LCK01-J.+Do+not+synchronize+on+objects+that+may+be+reused
> That said :-), this has two possible impacts of more immediate concern: memory model and maintainability.
> 1) The memory model only guarantees publication of final fields that are not safely published -- this is highly desired for fields used as lock objects because they may be the mechanism for safe publication. Yes it might work, but as my buddy Brian Goetz says, Just because you ran around with holding scissors all last week and didn't get hurt, is not a good rational to keep doing it.
> 2) The CERT "rule" is about mutation -- the final flag helps to clarify that the reference to the object used as a log is not intended to be changed. Avoiding a change that breaks a locking model.
> I also changed some lock objects from new String("foo") to new Object() (see patch), could be reverted (but left final) if there is some reason for the use of the larger String object. There was none I could see.
> I'm trying to verify a large portion of the lock usage statically and use a dynamic tool (SureLogic Flashlight) to check other aspects, but this was a simple patch I could provide right away as a small improvement.
> I have a patch, will attach (not a Jira expert yet)

--
This message is automatically generated by JIRA.
If you think it was sent incorrectly, please contact your JIRA administrators
For more information on JIRA, see: http://www.atlassian.com/software/jira