[Ironruby-core] Code Review: Thread#raise

Jim Deville jdeville at microsoft.com
Tue Jan 6 15:42:04 EST 2009


Um... other than meta programming (use an if $VERBOSE block to warn, then redefine the current method to remove the $VERBOSE block)

JD

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Shri Borde
Sent: Tuesday, January 06, 2009 12:25 PM
To: ironruby-core at rubyforge.org
Cc: Tomas Matousek
Subject: Re: [Ironruby-core] Code Review: Thread#raise

Is there a way to get the warning printed just the first time it was executed in the process? Uses like the get_unique_cookie example below would be called multiple times, and it would be extremely annoying to print the warning everytime the dangerous API was called.

We could build some internal infrastructure to implement this, but does Ruby have built-in support we could leverage?

Thanks,
Shri


-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Jim Deville
Sent: Tuesday, January 06, 2009 12:17 PM
To: ironruby-core at rubyforge.org
Cc: Tomas Matousek
Subject: Re: [Ironruby-core] Code Review: Thread#raise

-W _level_ sets the warning level. Internally, it sets $VERBOSE to true if -W, -W1 sets $VERBOSE to false and -W0 sets $VERBOSE to nil. If $VERBOSE is nil, no warnings (even those called by Kernel.warn) will be printed.

From what I can gather, -v, --version, -w and -W (-W with no level) are equivalent and all set $VERBOSE to true.

Charlie or Tomas, can you expand on that, or correct me if my understanding is incorrect?

JD

-----Original Message-----
From: ironruby-core-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Shri Borde
Sent: Tuesday, January 06, 2009 11:50 AM
To: ironruby-core at rubyforge.org
Cc: Tomas Matousek
Subject: Re: [Ironruby-core] Code Review: Thread#raise

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-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Charles Oliver Nutter
Sent: Tuesday, January 06, 2009 10:53 AM
To: ironruby-core at 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 at 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 at 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 at 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 at rubyforge.org
> http://rubyforge.org/mailman/listinfo/ironruby-core

_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core


More information about the Ironruby-core mailing list