Re: Change to ThreadBase for review

Thanks for the comments.
I agree with them and I'm used to doing Linux development via patches so
the splitting & white space comments make sense.
I wasn't expecting speedy responses. I probably would have waited a week
and then done what you suggest, commit and track comments.

The run state is only being used internally to track whether Wait()
and such need to clean up state. Since the clean up needs to happen
whether or not the thread has exited, the IsRunning() call would
now just indicate whether it was started or not.
The actual use is that all the callers call Wait() to be sure
a thread is finished.

On Tue, 2013-08-06 at 11:56 -0400, Eric Sunshine wrote:
> 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
> _______________________________________________
> Crystal-develop mailing list
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/crystal-develop



------------------------------------------------------------------------------
Get 100% visibility into Java/.NET code with AppDynamics Lite!
It's a free troubleshooting tool designed for production.
Get down to code-level detail for bottlenecks, with <2% overhead. 
Download for free and get started troubleshooting in minutes. 
http://pubads.g.doubleclick.net/gampad/clk?id=48897031&iu=/4140/ostg.clktrk