ThreadPool calling Thread.setName

[Peter Firmstone <ji...@zeus.net.au>]

Hope you don't mind, I've removed the call to Thread.setName in 
com.sun.jini.ThreadPool

As a result threads will be less descriptive, unfortunately setName 
isn't thread safe, it's final and cannot be overridden.  Thread.getName 
is only thread safe if a Thread's name isn't changed after publication.

ThreadPool was the only instance of Thread.setName in River.

Regards,

Peter.

Re: ThreadPool calling Thread.setName

[Peter <ji...@zeus.net.au>]

Based on your reasoning and because all tests are passing reliably without failure now, it should be safe to uncomment it.

Peter.


----- Original message -----
> I also don't see the particular concern with toString(). It uses
> getName(), so it does all its work with a particular char[]. At the
> worst, that char[] will either be an old one, or the current one but
> with some out-of-date elements, resulting in an unhelpful value.
>
> Patricia
>
>
> On 4/27/2013 5:55 AM, Patricia Shanahan wrote:
> > I'll read the paper. However, C and C++ tend to have nastier semantics
> > for data races than Java, which lacks anything quite as drastic as the C
> > definition of "undefined behavior".
> >
> > My reasoning for the relative safety of this particular race is based on
> > one of the more basic rules, atomicity of reference read or write,
> > combined with reading the relevant code. Regardless of data races, the
> > result of accessing "name" will always be a pointer to a char[]. We have
> > a significant chance of the char[] containing useful, helpful data in
> > the old version of the code. The probability of it containing anything
> > useful is zero in the new version.
> >
> > Patricia
> >
> >
> > On 4/27/2013 3:51 AM, Peter Firmstone wrote:
> > > Ok found the link, the paper's called:
> > > Position paper: Nondeterminism is unavoidable, but data races are
> > > pure evil
> > >
> > > http://www.hpl.hp.com/techreports/2012/HPL-2012-218.pdf
> > >
> > >
> > > Wheather Thread's name field is an issue or not depends on the code that
> > > uses it (Object.toString() and Thread.getName() methods do) and what
> > > that code does with the information.
> > >
> > > Outrigger uses it in lifecycle logs, although it doesn't appear to be
> > > called on any threads from ThreadPool.  Instead Outrigger calls
> > > getName() on threads it references from fields in OutriggerServerImpl.
> > > These fields have since been changed to final, so if the name is set
> > > during construction and not changed after publication, it's thread safe,
> > > in addition these threads aren't started until after
> > > OutriggerServerImpl's constructor returns.
> > >
> > > The concurrency issues I most recently experienced were related to
> > > Outrigger, I couldn't determine the cause of these failures, perhaps I
> > > lack sufficient ability to reason deeply about these issues, my fix for
> > > the problem was akin to carpet bombing; fix every concurrency problem
> > > and that appears to have paid off.
> > >
> > > It's very difficult to determine if toString() may be called on
> > > ThreadPool threads.  I reasoned it could be and that was sufficient
> > > justification to not use setName().
> > >
> > > Cheers,
> > >
> > > Peter.
> > >
> > >
> > >
> > >
> > >
> > >
> > > On 27/04/2013 1:20 AM, Patricia Shanahan wrote:
> > > > I've looked at the source code. The field "name" that is shared
> > > > between threads doing getName or setName on a given Thread is a
> > > > reference. Writes and reads of references are always atomic.
> > > >
> > > > The worst that could happen is that the change to the name does not
> > > > propagate to all threads that might display information about the
> > > > thread. The proposed fix ensures that worst case outcome happens all
> > > > the time.
> > > >
> > > > Patricia
> > > >
> > > > On 4/26/2013 5:44 AM, Greg Trasuk wrote:
> > > > >
> > > > > I'm curious, as I don't see any indication in the Javadocs that
> > > > > setName() isn't thread safe.  Is there another reference that calls
> > > > > that
> > > > > out?  And what would be the failure mode, apart from a mangled
> > > > > string in
> > > > > a log output?
> > > > >
> > > > > Personally, if the potential failure mode wasn't onerous, I'd opt for
> > > > > more descriptive logging.  Comprehensibility is everything when you're
> > > > > troubleshooting.
> > > > >
> > > > > Cheers,
> > > > >
> > > > > Greg.
> > > > >
> > > > > On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
> > > > > > Hope you don't mind, I've removed the call to Thread.setName in
> > > > > > com.sun.jini.ThreadPool
> > > > > >
> > > > > > As a result threads will be less descriptive, unfortunately setName
> > > > > > isn't thread safe, it's final and cannot be overridden.
> > > > > > Thread.getName
> > > > > > is only thread safe if a Thread's name isn't changed after
> > > > > > publication.
> > > > > >
> > > > > > ThreadPool was the only instance of Thread.setName in River.
> > > > > >
> > > > > > Regards,
> > > > > >
> > > > > > Peter.
> > > > >
> > > >
> > >
> >
>

Re: ThreadPool calling Thread.setName

[Patricia Shanahan <pa...@acm.org>]

I also don't see the particular concern with toString(). It uses 
getName(), so it does all its work with a particular char[]. At the 
worst, that char[] will either be an old one, or the current one but 
with some out-of-date elements, resulting in an unhelpful value.

Patricia


On 4/27/2013 5:55 AM, Patricia Shanahan wrote:
> I'll read the paper. However, C and C++ tend to have nastier semantics
> for data races than Java, which lacks anything quite as drastic as the C
> definition of "undefined behavior".
>
> My reasoning for the relative safety of this particular race is based on
> one of the more basic rules, atomicity of reference read or write,
> combined with reading the relevant code. Regardless of data races, the
> result of accessing "name" will always be a pointer to a char[]. We have
> a significant chance of the char[] containing useful, helpful data in
> the old version of the code. The probability of it containing anything
> useful is zero in the new version.
>
> Patricia
>
>
> On 4/27/2013 3:51 AM, Peter Firmstone wrote:
>> Ok found the link, the paper's called:
>> Position paper: Nondeterminism is unavoidable, but data races are
>> pure evil
>>
>> http://www.hpl.hp.com/techreports/2012/HPL-2012-218.pdf
>>
>>
>> Wheather Thread's name field is an issue or not depends on the code that
>> uses it (Object.toString() and Thread.getName() methods do) and what
>> that code does with the information.
>>
>> Outrigger uses it in lifecycle logs, although it doesn't appear to be
>> called on any threads from ThreadPool.  Instead Outrigger calls
>> getName() on threads it references from fields in OutriggerServerImpl.
>> These fields have since been changed to final, so if the name is set
>> during construction and not changed after publication, it's thread safe,
>> in addition these threads aren't started until after
>> OutriggerServerImpl's constructor returns.
>>
>> The concurrency issues I most recently experienced were related to
>> Outrigger, I couldn't determine the cause of these failures, perhaps I
>> lack sufficient ability to reason deeply about these issues, my fix for
>> the problem was akin to carpet bombing; fix every concurrency problem
>> and that appears to have paid off.
>>
>> It's very difficult to determine if toString() may be called on
>> ThreadPool threads.  I reasoned it could be and that was sufficient
>> justification to not use setName().
>>
>> Cheers,
>>
>> Peter.
>>
>>
>>
>>
>>
>>
>> On 27/04/2013 1:20 AM, Patricia Shanahan wrote:
>>> I've looked at the source code. The field "name" that is shared
>>> between threads doing getName or setName on a given Thread is a
>>> reference. Writes and reads of references are always atomic.
>>>
>>> The worst that could happen is that the change to the name does not
>>> propagate to all threads that might display information about the
>>> thread. The proposed fix ensures that worst case outcome happens all
>>> the time.
>>>
>>> Patricia
>>>
>>> On 4/26/2013 5:44 AM, Greg Trasuk wrote:
>>>>
>>>> I'm curious, as I don't see any indication in the Javadocs that
>>>> setName() isn't thread safe.  Is there another reference that calls
>>>> that
>>>> out?  And what would be the failure mode, apart from a mangled
>>>> string in
>>>> a log output?
>>>>
>>>> Personally, if the potential failure mode wasn't onerous, I'd opt for
>>>> more descriptive logging.  Comprehensibility is everything when you're
>>>> troubleshooting.
>>>>
>>>> Cheers,
>>>>
>>>> Greg.
>>>>
>>>> On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
>>>>> Hope you don't mind, I've removed the call to Thread.setName in
>>>>> com.sun.jini.ThreadPool
>>>>>
>>>>> As a result threads will be less descriptive, unfortunately setName
>>>>> isn't thread safe, it's final and cannot be overridden.
>>>>> Thread.getName
>>>>> is only thread safe if a Thread's name isn't changed after
>>>>> publication.
>>>>>
>>>>> ThreadPool was the only instance of Thread.setName in River.
>>>>>
>>>>> Regards,
>>>>>
>>>>> Peter.
>>>>
>>>
>>
>

Re: ThreadPool calling Thread.setName

[Patricia Shanahan <pa...@acm.org>]

I'll read the paper. However, C and C++ tend to have nastier semantics 
for data races than Java, which lacks anything quite as drastic as the C 
definition of "undefined behavior".

My reasoning for the relative safety of this particular race is based on 
one of the more basic rules, atomicity of reference read or write, 
combined with reading the relevant code. Regardless of data races, the 
result of accessing "name" will always be a pointer to a char[]. We have 
a significant chance of the char[] containing useful, helpful data in 
the old version of the code. The probability of it containing anything 
useful is zero in the new version.

Patricia


On 4/27/2013 3:51 AM, Peter Firmstone wrote:
> Ok found the link, the paper's called:
> Position paper: Nondeterminism is unavoidable, but data races are
> pure evil
>
> http://www.hpl.hp.com/techreports/2012/HPL-2012-218.pdf
>
>
> Wheather Thread's name field is an issue or not depends on the code that
> uses it (Object.toString() and Thread.getName() methods do) and what
> that code does with the information.
>
> Outrigger uses it in lifecycle logs, although it doesn't appear to be
> called on any threads from ThreadPool.  Instead Outrigger calls
> getName() on threads it references from fields in OutriggerServerImpl.
> These fields have since been changed to final, so if the name is set
> during construction and not changed after publication, it's thread safe,
> in addition these threads aren't started until after
> OutriggerServerImpl's constructor returns.
>
> The concurrency issues I most recently experienced were related to
> Outrigger, I couldn't determine the cause of these failures, perhaps I
> lack sufficient ability to reason deeply about these issues, my fix for
> the problem was akin to carpet bombing; fix every concurrency problem
> and that appears to have paid off.
>
> It's very difficult to determine if toString() may be called on
> ThreadPool threads.  I reasoned it could be and that was sufficient
> justification to not use setName().
>
> Cheers,
>
> Peter.
>
>
>
>
>
>
> On 27/04/2013 1:20 AM, Patricia Shanahan wrote:
>> I've looked at the source code. The field "name" that is shared
>> between threads doing getName or setName on a given Thread is a
>> reference. Writes and reads of references are always atomic.
>>
>> The worst that could happen is that the change to the name does not
>> propagate to all threads that might display information about the
>> thread. The proposed fix ensures that worst case outcome happens all
>> the time.
>>
>> Patricia
>>
>> On 4/26/2013 5:44 AM, Greg Trasuk wrote:
>>>
>>> I'm curious, as I don't see any indication in the Javadocs that
>>> setName() isn't thread safe.  Is there another reference that calls that
>>> out?  And what would be the failure mode, apart from a mangled string in
>>> a log output?
>>>
>>> Personally, if the potential failure mode wasn't onerous, I'd opt for
>>> more descriptive logging.  Comprehensibility is everything when you're
>>> troubleshooting.
>>>
>>> Cheers,
>>>
>>> Greg.
>>>
>>> On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
>>>> Hope you don't mind, I've removed the call to Thread.setName in
>>>> com.sun.jini.ThreadPool
>>>>
>>>> As a result threads will be less descriptive, unfortunately setName
>>>> isn't thread safe, it's final and cannot be overridden.  Thread.getName
>>>> is only thread safe if a Thread's name isn't changed after publication.
>>>>
>>>> ThreadPool was the only instance of Thread.setName in River.
>>>>
>>>> Regards,
>>>>
>>>> Peter.
>>>
>>
>

Re: ThreadPool calling Thread.setName

[Peter Firmstone <ji...@zeus.net.au>]

Ok found the link, the paper's called:
Position paper: Nondeterminism is unavoidable, but data races are
pure evil

http://www.hpl.hp.com/techreports/2012/HPL-2012-218.pdf


Wheather Thread's name field is an issue or not depends on the code that 
uses it (Object.toString() and Thread.getName() methods do) and what 
that code does with the information.

Outrigger uses it in lifecycle logs, although it doesn't appear to be 
called on any threads from ThreadPool.  Instead Outrigger calls 
getName() on threads it references from fields in OutriggerServerImpl.  
These fields have since been changed to final, so if the name is set 
during construction and not changed after publication, it's thread safe, 
in addition these threads aren't started until after 
OutriggerServerImpl's constructor returns.

The concurrency issues I most recently experienced were related to 
Outrigger, I couldn't determine the cause of these failures, perhaps I 
lack sufficient ability to reason deeply about these issues, my fix for 
the problem was akin to carpet bombing; fix every concurrency problem 
and that appears to have paid off.

It's very difficult to determine if toString() may be called on 
ThreadPool threads.  I reasoned it could be and that was sufficient 
justification to not use setName().

Cheers,

Peter.






On 27/04/2013 1:20 AM, Patricia Shanahan wrote:
> I've looked at the source code. The field "name" that is shared 
> between threads doing getName or setName on a given Thread is a 
> reference. Writes and reads of references are always atomic.
>
> The worst that could happen is that the change to the name does not 
> propagate to all threads that might display information about the 
> thread. The proposed fix ensures that worst case outcome happens all 
> the time.
>
> Patricia
>
> On 4/26/2013 5:44 AM, Greg Trasuk wrote:
>>
>> I'm curious, as I don't see any indication in the Javadocs that
>> setName() isn't thread safe.  Is there another reference that calls that
>> out?  And what would be the failure mode, apart from a mangled string in
>> a log output?
>>
>> Personally, if the potential failure mode wasn't onerous, I'd opt for
>> more descriptive logging.  Comprehensibility is everything when you're
>> troubleshooting.
>>
>> Cheers,
>>
>> Greg.
>>
>> On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
>>> Hope you don't mind, I've removed the call to Thread.setName in
>>> com.sun.jini.ThreadPool
>>>
>>> As a result threads will be less descriptive, unfortunately setName
>>> isn't thread safe, it's final and cannot be overridden.  Thread.getName
>>> is only thread safe if a Thread's name isn't changed after publication.
>>>
>>> ThreadPool was the only instance of Thread.setName in River.
>>>
>>> Regards,
>>>
>>> Peter.
>>
>

Re: ThreadPool calling Thread.setName

[Peter Firmstone <ji...@zeus.net.au>]

There's an interesting paper submitted to the concurrency interest mail 
list explaining why data races are bad. 

I'll go over the list and find the link for you.

Basically there are consequences worse than semantic failure alone.

The river code itself doesn't attempt to log the thread's name, but it 
is useful, although not essential in thread dumps.

Yes, it's a trade off.

Regards,

Peter.

Patricia Shanahan wrote:
> I've looked at the source code. The field "name" that is shared 
> between threads doing getName or setName on a given Thread is a 
> reference. Writes and reads of references are always atomic.
>
> The worst that could happen is that the change to the name does not 
> propagate to all threads that might display information about the 
> thread. The proposed fix ensures that worst case outcome happens all 
> the time.
>
> Patricia
>
> On 4/26/2013 5:44 AM, Greg Trasuk wrote:
>>
>> I'm curious, as I don't see any indication in the Javadocs that
>> setName() isn't thread safe.  Is there another reference that calls that
>> out?  And what would be the failure mode, apart from a mangled string in
>> a log output?
>>
>> Personally, if the potential failure mode wasn't onerous, I'd opt for
>> more descriptive logging.  Comprehensibility is everything when you're
>> troubleshooting.
>>
>> Cheers,
>>
>> Greg.
>>
>> On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
>>> Hope you don't mind, I've removed the call to Thread.setName in
>>> com.sun.jini.ThreadPool
>>>
>>> As a result threads will be less descriptive, unfortunately setName
>>> isn't thread safe, it's final and cannot be overridden.  Thread.getName
>>> is only thread safe if a Thread's name isn't changed after publication.
>>>
>>> ThreadPool was the only instance of Thread.setName in River.
>>>
>>> Regards,
>>>
>>> Peter.
>>
>
>

Re: ThreadPool calling Thread.setName

[Patricia Shanahan <pa...@acm.org>]

I've looked at the source code. The field "name" that is shared between 
threads doing getName or setName on a given Thread is a reference. 
Writes and reads of references are always atomic.

The worst that could happen is that the change to the name does not 
propagate to all threads that might display information about the 
thread. The proposed fix ensures that worst case outcome happens all the 
time.

Patricia

On 4/26/2013 5:44 AM, Greg Trasuk wrote:
>
> I'm curious, as I don't see any indication in the Javadocs that
> setName() isn't thread safe.  Is there another reference that calls that
> out?  And what would be the failure mode, apart from a mangled string in
> a log output?
>
> Personally, if the potential failure mode wasn't onerous, I'd opt for
> more descriptive logging.  Comprehensibility is everything when you're
> troubleshooting.
>
> Cheers,
>
> Greg.
>
> On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
>> Hope you don't mind, I've removed the call to Thread.setName in
>> com.sun.jini.ThreadPool
>>
>> As a result threads will be less descriptive, unfortunately setName
>> isn't thread safe, it's final and cannot be overridden.  Thread.getName
>> is only thread safe if a Thread's name isn't changed after publication.
>>
>> ThreadPool was the only instance of Thread.setName in River.
>>
>> Regards,
>>
>> Peter.
>

Re: ThreadPool calling Thread.setName

[Greg Trasuk <tr...@stratuscom.com>]

I'm curious, as I don't see any indication in the Javadocs that
setName() isn't thread safe.  Is there another reference that calls that
out?  And what would be the failure mode, apart from a mangled string in
a log output?

Personally, if the potential failure mode wasn't onerous, I'd opt for
more descriptive logging.  Comprehensibility is everything when you're
troubleshooting.

Cheers,

Greg.

On Fri, 2013-04-26 at 05:48, Peter Firmstone wrote:
> Hope you don't mind, I've removed the call to Thread.setName in 
> com.sun.jini.ThreadPool
> 
> As a result threads will be less descriptive, unfortunately setName 
> isn't thread safe, it's final and cannot be overridden.  Thread.getName 
> is only thread safe if a Thread's name isn't changed after publication.
> 
> ThreadPool was the only instance of Thread.setName in River.
> 
> Regards,
> 
> Peter.