accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Halloran (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-853) Fields and parameters that are used as locks change to be final (where possible)
Date Wed, 14 Nov 2012 02:26:11 GMT

    [ 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

Mime
View raw message