I asked ruby-core whether descheduling other threads is by spec, but no one other than you replied. Any suggestions on how to nudge others to consider it? If you want to reply to that thread and provide JRuby's vote of confidence for the idea, that might help.
We should nail the complete behavior before taking it to ruby-core. I have written up a draft at http://blogs.msdn.com/shrib/archive/2009/01/07/proposed-spec-for-ruby-s-thread-critical.aspx. Please take a look and comment on whether it works for JRuby. We can move it to some wiki if we need to iterate on it... Thanks, Shri -----Original Message----- From: ironruby-core-boun...@rubyforge.org [mailto:ironruby-core-boun...@rubyforge.org] On Behalf Of Charles Oliver Nutter Sent: Wednesday, January 07, 2009 5:12 AM To: ironruby-core@rubyforge.org Cc: Tomas Matousek Subject: Re: [Ironruby-core] Code Review: Thread#raise I think I'm coming around to your side. My position is current the following: 1. If critical officially remains specified as its current behavior in MRI, we should be warning people not to use it and should have a warning emitted at least once. 2. If we can get ruby-core to agree that critical should be expected to be no more than a global mutex and not necessarily deschedule threads, I'd be willing to make the same change in JRuby and omit the warning. I definitely think we need to talk to ruby-core and open it up to the community, however, to see if there are cases where people depend on critical= actually descheduling. Shri: can you propose making the "global mutex" definition of critical the official one? - Charlie Shri Borde wrote: > I agree that critical= can cause problems, and should go away. However, it > can also be used in a reasonably safe way. Something like this. Such usages > do not rely on other threads being frozen. Most uses seem to use such a > pattern. > > def get_unique_cookie() > begin > Thread.critical = true > cookie = @@counter > @@counter += 1 > ensure > Thread.critical = false > end > cookie > end > > I am suggesting not adding a warning purely from a pragmatic perspective. The > patter above is used in a lots of of places, and a warning could irritate > users if they see it too often when using their favorite gems. > > Is there a way for Ruby devs to control the warning level? If so, it will be > a question of what level to put the warning under, not whether to add it or > not. > > Thanks, > Shri > > > -----Original Message----- > From: ironruby-core-boun...@rubyforge.org > [mailto:ironruby-core-boun...@rubyforge.org] On Behalf Of Charles Oliver > Nutter > Sent: Tuesday, January 06, 2009 10:53 AM > To: ironruby-core@rubyforge.org > Cc: IronRuby External Code Reviewers; Tomas Matousek > Subject: Re: [Ironruby-core] Code Review: Thread#raise > > I think the implications of critical= in MRI are more complicated than > just a local lock. It actually stops normal thread scheduling, so other > threads freeze what they're doing. That's vastly more intrusive than > just a lock or critical section: > > ◆ ruby -e "Thread.new { sleep 1; puts Time.now; redo }; sleep 3; > Thread.critical = true; puts 'critical'; sleep 3; puts 'leaving > critical'; Thread.critical = false; sleep 3" > Tue Jan 06 18:50:34 +0000 2009 > Tue Jan 06 18:50:35 +0000 2009 > critical > leaving critical > Tue Jan 06 18:50:39 +0000 2009 > Tue Jan 06 18:50:40 +0000 2009 > Tue Jan 06 18:50:41 +0000 2009 > > If those other threads are holding locks or resources, deadlock is > extremely easy, and of course it's nearly impossible to emulate right > with real parallel threads since you can't easily stop them whenever you > want. > > #kill and #raise are also very tricky and dangerous, but they're at > least localized. They can be defined in terms of a message passed from > one thread to another plus a periodic message checkpoint. > > I still believe critical= is much more in need of a warning. > > Shri Borde wrote: >> Warning sounds reasonable for Thread#kill and Thread#raise. FWIW, Mongrel >> and webbrick do use Thread#kill to kill a background thread. >> >> Thread.critical= can actually be used in a sane way for simple >> synchronization, like the lock keyword in C#. This is used much more widely, >> and a warning for this will cause lot of false alarms. >> >> Thanks, >> Shri >> >> >> -----Original Message----- >> From: Tomas Matousek >> Sent: Saturday, January 03, 2009 11:52 AM >> To: Curt Hagenlocher; Shri Borde; IronRuby External Code Reviewers >> Cc: ironruby-core@rubyforge.org >> Subject: RE: Code Review: Thread#raise >> >> I think we should at least report a warning when Thread#kill, Thread#raise >> or Thread#critical= is called if not eliminating them as Charlie proposed on >> his blog. >> >> Tomas >> >> -----Original Message----- >> From: Curt Hagenlocher >> Sent: Tuesday, December 30, 2008 10:05 PM >> To: Shri Borde; IronRuby External Code Reviewers >> Cc: ironruby-core@rubyforge.org >> Subject: RE: Code Review: Thread#raise >> >> Shouldn't the option "UseThreadAbortForSyncRaise" be called >> "...ForASyncRaise"? >> I think that Thread.raise with no arguments should just inject a >> RuntimeError with no message as if $! were nil; this makes more sense than >> failing. Trying to reference a "current exception" in another thread is a >> scary operation even if that's what MRI is doing. >> >> Other than that, changes look really nice. But anyone thinking of using >> this functionality should read Charlie's excellent piece from earlier in the >> year: >> http://blog.headius.com/2008/02/rubys-threadraise-threadkill-timeoutrb.html >> >> -----Original Message----- >> From: Shri Borde >> Sent: Monday, December 29, 2008 3:00 PM >> To: IronRuby External Code Reviewers >> Cc: ironruby-core@rubyforge.org >> Subject: Code Review: Thread#raise >> >> tfpt review "/shelveset:raise;REDMOND\sborde" >> Comment : >> Implements Thread#raise using Thread.Abort, and added tests for it >> Implemented stress mode (RubyOptions.UseThreadAbortForSyncRaise) which >> found more issues. Fixed most but not all >> Enabled test for timeout as well >> Remaining work (not high pri for now) >> - Thread#raise without Exception parameters is not supported as it needs >> to access the active exception of the target thread. This is stored as a >> thread-local static, and so cannot be accessed from other threads. Can be >> fixed by not using ThreadStaticAttribute. >> - Adding probes (in generated code, in C# library code, etc) will help to >> raise the exception quicker as Thread.Abort can be delayed indefinitely. >> Ideally, we need both approaches. For now, using Thread.Abort goes a long >> way. >> - Ideally, we would add a try-catch to the IDynamicObject/MetaObject code >> paths so that when other languages called into Ruby code, they would get the >> expected user exception rather than a ThreadAbortException >> >> RunRSpec: supports -ruby to run with MRI. Its much faster than doing "rake >> ruby why_regression". Added support for -e to run a specific example >> >> >> _______________________________________________ >> Ironruby-core mailing list >> Ironruby-core@rubyforge.org >> http://rubyforge.org/mailman/listinfo/ironruby-core > > _______________________________________________ > Ironruby-core mailing list > Ironruby-core@rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core > _______________________________________________ > Ironruby-core mailing list > Ironruby-core@rubyforge.org > http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list Ironruby-core@rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core _______________________________________________ Ironruby-core mailing list Ironruby-core@rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core
