[PATCH] Fixes for rb_thread_critical bugs

[Brent Roman]

I recently was involved in trying to get a multi-threaded ruby motion
control application to handle a SIGINT trap reliably. After digging
around a bit in eval.c, it became clear that:

1) Ruby trap handlers always run on Thread.main

2) Ruby trap handlers may be entered even if Thread.critical is set.

Most of my trouble stemmed from not realizing that traps always ran on
Thread.main. Once I was explicit about the thread(s) in which the trap
handler raised exceptions, everything become _MUCH_ more predicable.
In retrospect, I suppose this behavior should have been obvious

(Is it documented?)

But, I could not find a way to script around the fact that Ruby trap
handlers may interrupt Ruby threads in critical sections. I suppose
that the intent here was to insure that ruby didn't "lock up" in a
poorly coded critical section. However, if one can abort a
critical section, how can one recover from that interruption and
continue processing? Strictly speaking, once a critical section
is aborted, one must assume that the data
that section manipulates are corrupted.

Unless I am missing something fundamental here, one must
modify the Ruby interpreter such that no asynchronous event can
interrupt ruby Thread while Thread.critical is set. This means that
a thread that spins in a critical section can cause Ruby to cease
responding to traps. I feel this is a small price to pay for the
ability to handle traps deterministically.

Another problem I noticed while poking around in the code was that
rb_thread_schedule() is called in a couple places without first checking
whether rb_thread_critical is set. For instance, Thread.pass from a
critical thread can cause the woken thread to become critical.
This cannot be the desired behavior.

(Or, again, am I missing something?)

Thread.pass _should_ behave like Thread.stop. The calling thread clears
Thread.critical and reschedules in one atomic operation.

Even single threaded scripts may need to shield themselves against
taking exceptions at inappropriate times. The 'C' code does this by
setting rb_prohibit_interrupt, but I cannot find any ruby method for
this.. So, these patches also prohibit trap evaluation when the Thread
is critical.

I don't think any of these changes should affect sanely designed
multi-threading scripts. Any scripts that rely on the current behavior
are working due, mostly, to luck.

These patches are against the 1.6.8 release.
I believe none of this has been changed in the 1.8.x series,
but I have not looked closely. Please let me know what you
think of this.

Should this be posted to Ruby-Core?

Thanks.


-----

[eval.c]
6c6
< $Date: 2003/02/11 20:56:01 $
---

$Date: 2004/12/20 05:17:18 $

5365d5364
< rb_thread_schedule();
7706c7757
< rb_thread_pending = 0;
---

rb_thread_pending = rb_thread_critical = 0;

8231d8281
< rb_thread_critical = 0;


[regex.c]
@@ -65,11 +65,11 @@
#include "defines.h"

# define RUBY
-extern int rb_prohibit_interrupt;
+extern int rb_prohibit_interrupt, rb_thread_critical;
extern int rb_trap_pending;
void rb_trap_exec _((void));

-# define CHECK_INTS if (!rb_prohibit_interrupt) {\
+# define CHECK_INTS if (!(rb_prohibit_interrupt | rb_thread_critical)) {\
if (rb_trap_pending) rb_trap_exec();\
}


[rubysig.h]
@@ -66,22 +66,20 @@
void rb_thread_schedule _((void));
#if defined(HAVE_SETITIMER) && !defined(__BOW__)
EXTERN int rb_thread_pending;
-# define CHECK_INTS if (!rb_prohibit_interrupt) {\
+# define CHECK_INTS if (!(rb_prohibit_interrupt | rb_thread_critical)) {\
+ if (rb_thread_pending) rb_thread_schedule();\
if (rb_trap_pending) rb_trap_exec();\
- if (rb_thread_pending && !rb_thread_critical) rb_thread_schedule();\
}
#else
/* pseudo preemptive thread switching */
EXTERN int rb_thread_tick;
#define THREAD_TICK 500
-#define CHECK_INTS if (!rb_prohibit_interrupt) {\
- if (rb_trap_pending) rb_trap_exec();\
- if (!rb_thread_critical) {\
+#define CHECK_INTS if (!rb_prohibit_interrupt && !rb_thread_critical) {\
if (rb_thread_tick-- <= 0) {\
rb_thread_tick = THREAD_TICK;\
rb_thread_schedule();\
}\
- }\
+ if (rb_trap_pending) rb_trap_exec();\
}
#endif

 

[Yukihiro Matsumoto]

Hi,

In message "Re: [PATCH] Fixes for rb_thread_critical bugs"

|Should this be posted to Ruby-Core?

Thank you for the proposal. Nonetheless, it should be posted to
ruby-core, and the patch should be standard unified diff.

matz.