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 -------------- next part -------------- A non-text attachment was scrubbed... Name: raise.diff Type: application/octet-stream Size: 34302 bytes Desc: raise.diff URL: <http://rubyforge.org/pipermail/ironruby-core/attachments/20081229/1f453997/attachment-0001.obj>
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
The terminology I am using throughout is synchronous exception would be a normal Kernel.raise as the thread knows exactly when and where the exception will occur. Thread#raise is considered to raise an exception asynchronously as you cannot control exactly when and where the exception will actually be raised on the target thread. So with this terminology, the stress mode should stay as "...ForSyncRaise" so that Thread.Abort is used even for Kernel.raise. I will change Thread.raise with no arguments to inject a RuntimeError. I am not sure if its any better or worse at it depends on whether its preferable to fail early or to try to keep going. Failing early is usually a good idea, but in this case, given all the caveats about Thread#raise, I don''t feel strongly. Thanks, Shri -----Original Message----- From: Curt Hagenlocher Sent: Tuesday, December 30, 2008 1: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
Ah, I see; I misunderstood the way that flag was working. -----Original Message----- From: Shri Borde Sent: Tuesday, December 30, 2008 1:23 PM To: Curt Hagenlocher; IronRuby External Code Reviewers Cc: ironruby-core at rubyforge.org Subject: RE: Code Review: Thread#raise The terminology I am using throughout is synchronous exception would be a normal Kernel.raise as the thread knows exactly when and where the exception will occur. Thread#raise is considered to raise an exception asynchronously as you cannot control exactly when and where the exception will actually be raised on the target thread. So with this terminology, the stress mode should stay as "...ForSyncRaise" so that Thread.Abort is used even for Kernel.raise. I will change Thread.raise with no arguments to inject a RuntimeError. I am not sure if its any better or worse at it depends on whether its preferable to fail early or to try to keep going. Failing early is usually a good idea, but in this case, given all the caveats about Thread#raise, I don''t feel strongly. Thanks, Shri -----Original Message----- From: Curt Hagenlocher Sent: Tuesday, December 30, 2008 1: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
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
kill and raise might be a bit of a bother if you add warnings, since they''re used in several libraries without any good replacement other than reimpl (usually to interrupt stuck IO operations). But yeah, I''d be on board with an across-the-board Thread.critical= warning in both JRuby and IronRuby. Tomas Matousek wrote:> 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
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
I replied before seeing this... Taking a sampling of a few gems, I think it should actually be the other way. Mongrel and webbrick do use Thread#kill to kill a background thread, but other gems do not. Thread#raise was also used in just a couple of places to raise a timeout error. And it is very hard, if not impossible, to use these APIs safely since you do not know what the other thread is doing. OTOH, Thread.critical= can actually be used in a sane way for simple synchronization, like the lock keyword in C#. This is used way more widely, and a warning for this will cause lot of false alarms. Thanks, Shri -----Original Message----- From: Charles.O.Nutter at sun.com [mailto:Charles.O.Nutter at sun.com] On Behalf Of Charles Oliver Nutter Sent: Monday, January 05, 2009 6:26 AM To: ironruby-core at rubyforge.org Cc: Curt Hagenlocher; Shri Borde; IronRuby External Code Reviewers Subject: Re: [Ironruby-core] Code Review: Thread#raise kill and raise might be a bit of a bother if you add warnings, since they''re used in several libraries without any good replacement other than reimpl (usually to interrupt stuck IO operations). But yeah, I''d be on board with an across-the-board Thread.critical= warning in both JRuby and IronRuby. Tomas Matousek wrote:> 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
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
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
-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
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
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
Nice, that made very little sense.
I was trying to say that I don''t know of a way other than redefining
the method after the first run.
JD
-----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:42 PM
To: ironruby-core at rubyforge.org
Cc: Tomas Matousek
Subject: Re: [Ironruby-core] Code Review: Thread#raise
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
_______________________________________________
Ironruby-core mailing list
Ironruby-core at rubyforge.org
http://rubyforge.org/mailman/listinfo/ironruby-core
No, Ruby doesn''t have any such warning filter. You can place a flag on
RubyContext (via GetOrCreateLibraryData) that remembers for the current runtime
that the warning has already been reported.
Tomas
-----Original Message-----
From: 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
Jim Deville wrote:> -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?Correct.
Tomas Matousek wrote:> No, Ruby doesn''t have any such warning filter. You can place a flag on RubyContext (via GetOrCreateLibraryData) that remembers for the current runtime that the warning has already been reported.I''d expect to do the same in JRuby; it would be a one-time warning for sure. Basically advice on first use of critical= that "you shouldn''t be doing that, but I''ll let you". - Charlie
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-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
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-bounces at rubyforge.org [mailto:ironruby-core-bounces at rubyforge.org] On Behalf Of Charles Oliver Nutter Sent: Wednesday, January 07, 2009 5:12 AM To: ironruby-core at 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-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
I missed you saying we should nail it first...I already responded on ruby-core. But I think I agree with everything except for a couple details: * I think Thread.stop should release the lock as it does now. That''s a pretty minor expectation, and since it''s already there (and I know at least one library depends on it) it should remain. * I''m dubious about the lock being reentrant. In JRuby I''ve implemented it using a ReentrantLock, which should count up and count down, but it''s not necessarily a requirement. However I think it would be bad if a thread could block by setting critical=true when it''s already critical. * I think setting critical=false should only release the lock iff it is held by the current thread, and do nothing otherwise. I also agree that a thread terminating while critical is undefined behavior. But I think most of these questions could be answered by treating critical= as a globally-shared reentrant mutex. Thoughts? Shri Borde wrote:> 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 for continuing the thread on ruby-core. Let''s see how it goes. There are couple of minor issues with Thread.stop releasing the lock: * The code will be hard to read as most people probably don''t realize that Kernel#stop releases the lock. * Currently, Kernel#sleep does not release the lock. At the least, it should be consistent with Thread.stop. * If the programmer wants to release the lock before stopping the thread, its trivial for the programmer to call Thread.critical=false It''s mostly a question of aesthetics and simplicity. The more Thread.critical= can be locked down, the more understandable it will be. But none of these are deal breakers, so I don''t feel too strongly. About reentrancy, I am proposing that it *not* be reentrant (unless an implementation choses to support that behavior). So I think we are on the same page. Reentrancy would normally have been a good thing, but since it looks like an assignment, it would read very weirdly in the case below. Thread.critical=true Thread.critical=true # if Thread.critical= was reentrant, this would increment the counter to 2 Thread.critical=false puts Thread.critical # would be weird if this printed true Thread.critical=false Allowing an arbitraty thread to do Thread.critical=false has the same issue as reentrancy. If Thread.critical stayed true after some thread did Thread.critical=false, it would be very misleading. Since the thread cannot actually set Thread.critical to false, it should be an error. About thread terminating while critical is undefined, some implementations may be able to know if the dying thread still holds the lock, but some may not. Especially in the hosted case where some program is using Ruby as a scripting engine, the hosting app may have created the thread and run some Ruby snippet on it which entered the global critical section, but forgot to leave it. In that case, it would be too burdensome to require that the Ruby implementation deal with this case. Hence, the proposal of leaving this undefined. 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: Monday, January 12, 2009 7:55 AM To: ironruby-core at rubyforge.org Subject: Re: [Ironruby-core] Behavior for Thread.critical I missed you saying we should nail it first...I already responded on ruby-core. But I think I agree with everything except for a couple details: * I think Thread.stop should release the lock as it does now. That''s a pretty minor expectation, and since it''s already there (and I know at least one library depends on it) it should remain. [Shri Borde] There are couple of issues with allowing this: Currently, Kernel#sleep does not release the lock. At the least, it should be consistent with Thread.stop. If the programmer wants to release the lock before * I''m dubious about the lock being reentrant. In JRuby I''ve implemented it using a ReentrantLock, which should count up and count down, but it''s not necessarily a requirement. However I think it would be bad if a thread could block by setting critical=true when it''s already critical. * I think setting critical=false should only release the lock iff it is held by the current thread, and do nothing otherwise. I also agree that a thread terminating while critical is undefined behavior. But I think most of these questions could be answered by treating critical= as a globally-shared reentrant mutex. Thoughts? Shri Borde wrote:> 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..._______________________________________________ Ironruby-core mailing list Ironruby-core at rubyforge.org http://rubyforge.org/mailman/listinfo/ironruby-core