Re: Change to ThreadBase for review

CS development is somewhat quiescent these days, so please do not feel
snubbed if people don't respond to code review requests immediately or
at all. If you feel confident about your changes, sometimes the most
pragmatic approach is to commit them and then monitor crystal-main,
crystal-develop, and crystal-tracker mailing lists for problem
reports. (Bug reports on Trac get posted to cryatal-tracker.)

Regarding this patch, it might be easier to review if split into
several pieces. For instance, there are a number of whitespace-only
changes which make the patch noisy, plus some other changes not
necessarily dependent upon each other, which could be split apart. For
instance, I could envision separate patches: (1) general whitespace
cleanup, (2) drop unnecessary pthread_attr in favor of default values,
(3) refactoring/simplification of pthread implementation, (4) ditto
for win32 implementation. If you're using git for development, then
it's pretty easy to organize your patches like this. If not, then
splitting the patch might be more hassle than it's worth.

More comments below...

On Mon, Aug 5, 2013 at 4:19 PM, Ralph Campbell <[email protected]> wrote:
> I started off running valgrind on some simple tests and the deeper I
> dug, the more dirt I found. :-)
> Basically, for each call to pthread_create(), there should be a matching
> call to pthread_join() to recover system resources.
> The current code for ThreadBase::Wait() checks IsRunning() to prevent
> thread_join() from being called before pthread_create() is called.
> Having the running/not-running state cleared by the child thread when
> runnable->Run() returns in proxyFunc() means that if the child has
> finnished, pthread_join() is never called.

Diagnosis makes sense.

> Looking at the implementation more closely, I decided to simplify things
> by removing the use of ThreadStartParams and Barrier, and making the
> isRunning flag be set only by Start().
> Or in other words, isRunning means threadHandle is valid.

This changes semantics a bit since we no longer report whether the
underlying pthread is still running. Do any of the existing clients in
CS care about this slight difference in behavior? Can we make a guess
about external clients?

Without making an exhaustive audit, my guess would be that the
semantic change should not matter to clients and that this change
should be safe.

> I think this change also allows the thread to be Stop()'ed and Start()'ed
> more than once although I haven't tested that.
>
> I made similar changes to the win32 threads and I'm hoping
> one of you will test it for me. :-)

If nobody steps forward, and if you can't test it yourself via virtual
machine, the above advice might be reasonable: commit and wait for
someone to complain. (It's not great advice, but...)

> If you can't apply it easily due to white space mangling, I can
> resend as a tar or zip file.

Overall, the approach taken by the patch seems sound enough, nor I did
spot any obvious flaws in my (perhaps less than thorough) review.

-- ES

------------------------------------------------------------------------------
Get your SQL database under version control now!
Version control is standard for application code, but databases havent 
caught up. So what steps can you take to put your SQL databases under 
version control? Why should you start doing it? Read more to find out.
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk