Atomically adding a column to columns_

[Ohad Shacham <oh...@gmail.com>]

Hi,



We are building a tool for detecting non atomic usages of concurrent
collections.



We analyzed your code using our tool and found the following behavior:



Function addColumn at class SuperColumn tries to atomically add a column to
the concurrent collection “columns_” using the following code:



IColumn oldColumn = columns_.putIfAbsent(name, column);
if (oldColumn != null)
{

IColumn reconciledColumn = reconciler.reconcile((Column)column,
(Column)oldColumn);

while (!columns_.replace(name, oldColumn, reconciledColumn))  {

oldColumn = columns_.get(name);
reconciledColumn = reconciler.reconcile((Column)column, (Column)oldColumn);

 }

}



This code can throw NullPointerException at function reconcile if there is a
columns_.remove(…) operation on the same key, by a different thread, between
the previous executed “columns_.replace(…)” and “columns_.get(…)”
operations.



Could you please let me know whether you consider this as a bug?



Another very similar scenario can occur at function “addColumn” of class
“ColumnFamily” where the program can throw NullPointerException while
checking the while loop condition.



A possible way of modifying the code for atomically adding a column to
“columns_” can be as follows:



for (;;) {

IColumn  oldColumn = columns_.get(name);



if (oldColumn == null) {

if (columns_.putIfAbsent(name, column) == null)

return;

} else {

IColumn reconciledColumn = reconciler.reconcile((Column)column,
(Column)oldColumn);



if (columns_.replace(name, oldColumn, reconciledColumn))

return;

}

}





Thanks,

Ohad

Re: Atomically adding a column to columns_

[Peter Schuller <pe...@infidyne.com>]

> It would be good to document this, or, since the
> correct-even-for-remove logic is not much more complicated, switch to
> that.

Submitted:

https://issues.apache.org/jira/browse/CASSANDRA-1559

-- 
/ Peter Schuller

Re: Atomically adding a column to columns_

[Jonathan Ellis <jb...@gmail.com>]

On Mon, Sep 27, 2010 at 1:54 PM, Peter Schuller
<pe...@infidyne.com> wrote:
> In the case of this bit of code, I believe, and I am not speaking
> authoritatively, the removal only happens in (1) the read path when
> filtering results (on presumably query-local data) and (2) during
> compaction (on presumably compaction-local data).

Right, remove should only be called on local copies rather than the
global memtable.

It would be good to document this, or, since the
correct-even-for-remove logic is not much more complicated, switch to
that.

-- 
Jonathan Ellis
Project Chair, Apache Cassandra
co-founder of Riptano, the source for professional Cassandra support
http://riptano.com

Re: Atomically adding a column to columns_

[Peter Schuller <pe...@infidyne.com>]

> Function addColumn at class SuperColumn tries to atomically add a column to
> the concurrent collection “columns_” using the following code:

Deletions in Cassandra involve an insertion of a tombstone rather than
actual column deletion.

In the case of this bit of code, I believe, and I am not speaking
authoritatively, the removal only happens in (1) the read path when
filtering results (on presumably query-local data) and (2) during
compaction (on presumably compaction-local data). I did browse through
the various callers which seemed to confirm that this was the case. In
other words, I do not believe the remove() code path should not ever
be taken concurrently with insertions (by design, and not by
accident).

Anyone care to confirm/deny?

-- 
/ Peter Schuller