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

From: flameframe_at_[hidden]
Date: 2001-09-07 03:39:31

--- 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'.

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.

>  
> > - 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.

> > - 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();
                }
.

> > - 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. :-))  

> > - 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. 
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'.

Best regards,
Vitalij.