thread review (was Re: New file uploaded to boost /)

From: williamkempf_at_[hidden]
Date: 2001-09-07 07:52:43

--- In boost_at_y..., flameframe_at_h... wrote:
> --- In boost_at_y..., williamkempf_at_h... wrote:
> > --- In boost_at_y..., flameframe_at_h... wrote:
> > > 
> > > Some minor comments about a submission of threadcorrect1.zip
> > > 
> > > - class thread_specific_ptr miss 'set' function which is 
required 
> > to 
> > > initialize it.
> > 
> > This is the purpose of reset(), as documented.
> > 
> Ok. Sorry I was mislead by example which uses 'set' function. 
> Probably it should be 'reset'.

Hmmm... I'll have to fix that.

> And, of course, 'initially storing a null pointer.' in constructor 
> postcondition solves the problem. But does Win32 implementation of 
> tss take care about this postcondition? Sorry if I overlooked that, 
> but there is no explicit setting value to 0 and Win32 doc does not 
> guarantee that slot is zero after allocation.

Win32 does gaurantee this.  From the doc for TlsGetValue (emphasis 
added by me):

Remarks

TLS indexes are typically allocated by the TlsAlloc function during 
process or DLL initialization. Once allocated, each thread of the 
process can use a TLS index to access its own TLS storage slot for 
that index. The storage slot for each thread is _initialized to 
NULL_. A thread specifies a TLS index in a call to TlsSetValue, to 
store a value in its slot. The thread specifies the same index in a 
subsequent call to TlsGetValue, to retrieve the stored value. 

> > > - documentation does not expose a drawback that on Win32 users 
of 
> > the 
> > > library must build and deliver with application an additional 
> > > threadmon.dll if they want to make use of thread_specific_ptr.
> > 
> > I'm not sure I consider this a drawback, nor do I think such an 
> > implementation detail needs documentation.
> 
> If customer expect _one_ module (exe or dll) and naive proogrammer 
> deliver a bunch of dlls the he will know what a drawback is... :-). 
> But it is not a serious issue - writing a self-cleaning version is 
> trivial.

Self-cleaning?

> > > - implementation of call_once will fail when called function 
> throws
> > 
> > I didn't think about this one.  I'm not sure if we should try 
> > and "fix" call_once or document that such functions must not 
throw.
> 
> Fix is easy - instead of 
> 		if (!flag)
> 		{
> 			func();
> 			flag = true;
> 		}
> should be
> 		if (!flag)
> 		{
> 			flag = true;
> 			func();
> 		}

Not so easy.  This creates a race condition.  Plus claiming that 
an "init method" has been run, when that method failed, can lead to 
hard to diagnose bugs.  I'm leaning towards the no throw 
documentation for this reason.

> > > - atomic type was discussed shortly and excluded from the 
> library. 
> > > reasons are unclear. there should be at least rationale in docs.
> > 
> > It's too low level and difficult to use properly (you run into 
> > problems with memory access issues similar to the problem with 
the 
> > DCL pattern when using this type for "atomic ref-counting", which 
> is 
> > the reason most folks want this type).  I'm not sure that 
> documenting 
> > the rationale for this is appropriate, since most users aren't 
> going 
> > to expect to find such a type any way (the only MT library I'm 
> aware 
> > of in general use that includes such a concept is Win32).  I'd 
> > consider writing a FAQ for this, but if we decide to add such a 
> > concept later (a distinct possibility) we'd have to remove the 
FAQ.
> >  
> 
> If it will not appear in docs, I will appreciate if you point me to 
> some sources adressing these memory access issues. (I always knew 
> there is some poltergeist crashing my programs. :-))  

On Win32 (IA32) there won't be a problem here.  It's on other 
platforms where memory issues can arise.

> > > - in my opinion the library must be logically splitted into 
> > > synchronization part and thread part. This will allow users to 
> > build 
> > > alternative thread designs like thread::ref without lost of 
other 
> > > functionality. 
> > 
> > I don't agree.  The synchronization types currently are in 
headers 
> > that do not include <boost/thread/thread.hpp>, so there's enough 
of 
> a 
> > logical split to allow such alternative designs today.  Further, 
> I'd 
> > expect such alternative designs to be built on top of 
boost::thread 
> > to begin with, to take advantage of the portability already built 
> in.
> >  
> 
> As in example mentioned before with thread.cpp needed to use mutex. 

That was an oversite and will be fixed.

> Boost libraries tends to interdependence. 
> And with alternative designs I do not mean something portable, to 
be 
> submitted to boost - just something in real-world e.g. specialized 
> for a specific platform due to, for example, performance 
> requirements. It will be a pity to be not able to use 
> boost::condition (as well as other synch. primitives) in 
independent 
> thread implementation - not 'built on top of'.

Nothing prevents this today (though there is an unecessary dependence 
causing thread.cpp to need included in the build even if 
boost::thread is never used, but like I said, I'll fix that).

Bill Kempf