Atomicity violation in setAttribute

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

Hi,



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



We analyzed Tomcat’s code using our tool and found the following behavior:



The following code is located at the beginning of function setAttribute at
class ApplicationContext. This code fragment intends to check whether a key
is already inside the “attributes” collection. If it does, then a “replaced”
bit is turned on while the corresponding value is kept in “oldValue”. Then a
new value is inserted for the same key and the function continue while using
“replaced” and “oldValue”.



oldValue = attributes.get(name);
if (oldValue != null)
    replaced = true;
attributes.put(name, value);



This code can behave non atomically when two different threads call
setAttribute function with the same attribute name. In this case, there
exists a thread interleaving where two threads call the get function before
both of them launch the put operation. In this case, both threads will
continue running the function with “replaced” bit turned on and the oldValue
of both thread will point to the same location. This means that the value of
the first thread that did the put operation will not be treated as a value
that was replaced.



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



A possible way of modifying the code in order to execute this code
atomically is as follows:



val = attributes.put(name, value);
if (val != null)
    replaced = true;



Thanks,

Ohad

Re: Atomicity violation in setAttribute

[Ohad Shacham <oh...@cs.tau.ac.il>]

Given that there is a violation, and the proposed solution is simpler, would
you take it in the next release?

Thanks,
Ohad

On Sun, Sep 26, 2010 at 10:42 PM, Tim Whittington <ti...@apache.org> wrote:

> This is technically a race condition, but given the vague information
> provided in ServletContextAttributeEvent (e.g. the inability to
> differentiate adds vs replaces) I can't see it causing a real problem.
> i.e. there's no atomicity in the interaction of an attribute listener
> with the context, so a temporary internal inconsistency isn't going to
> hurt.
>
> > val = attributes.put(name, value);
> > if (val != null)
> >    replaced = true;
>
> +1 - this is simpler in any case.
>
> cheers
> tim
>
> On Mon, Sep 27, 2010 at 6:25 AM, Ian Darwin <ia...@darwinsys.com> wrote:
> >>  In this case, both threads will
> >> continue running the function with ?replaced? bit turned on and the
> oldValue
> >> of both thread will point to the same location. This means that the
> value of
> >> the first thread that did the put operation will not be treated as a
> value
> >> that was replaced.
> >>
> >> Could you please let me know whether you consider this as a bug?
> >
> > Is it any different than if two threads invoke the operation serially?
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> > For additional commands, e-mail: dev-help@tomcat.apache.org
> >
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>

Re: Atomicity violation in setAttribute

[Tim Whittington <ti...@apache.org>]

This is technically a race condition, but given the vague information
provided in ServletContextAttributeEvent (e.g. the inability to
differentiate adds vs replaces) I can't see it causing a real problem.
i.e. there's no atomicity in the interaction of an attribute listener
with the context, so a temporary internal inconsistency isn't going to
hurt.

> val = attributes.put(name, value);
> if (val != null)
>    replaced = true;

+1 - this is simpler in any case.

cheers
tim

On Mon, Sep 27, 2010 at 6:25 AM, Ian Darwin <ia...@darwinsys.com> wrote:
>>  In this case, both threads will
>> continue running the function with ?replaced? bit turned on and the oldValue
>> of both thread will point to the same location. This means that the value of
>> the first thread that did the put operation will not be treated as a value
>> that was replaced.
>>
>> Could you please let me know whether you consider this as a bug?
>
> Is it any different than if two threads invoke the operation serially?
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

Re: Atomicity violation in setAttribute

[Ian Darwin <ia...@darwinsys.com>]

>  In this case, both threads will
> continue running the function with ?replaced? bit turned on and the oldValue
> of both thread will point to the same location. This means that the value of
> the first thread that did the put operation will not be treated as a value
> that was replaced.
> 
> Could you please let me know whether you consider this as a bug?

Is it any different than if two threads invoke the operation serially?

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org