Skip to content
This repository

Correctly implement Thread#kill #1882

Merged
merged 3 commits into from over 1 year ago

3 participants

Ryo Onodera Don't Add Me To Your Organization a.k.a The Travis Bot Dirkjan Bussink
Ryo Onodera
Collaborator

Re-implement Thread#kill by adding new RaiseReason rather than complicating the
existing ad-hoc way of raising Exceptions.

Conceputually, Thread#kill isn't quite like exceptions. If the exception approach
wasn't replaced, it would be neccesary to really specially handle Thread::Die.

Make some failing specs pass. Also, Fix #864.

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request fails (merged b39c19ad into 149a364).

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 8dbf02a4 into 149a364).

vm/builtin/thread.cpp
... ...
@@ -309,6 +309,29 @@ static intptr_t thread_debug_id(pthread_t thr) {
309 309
     return exc;
310 310
   }
311 311
 
  312
+  Object* Thread::kill(STATE, GCToken gct) {
  313
+    init_lock_.lock();
1
Dirkjan Bussink Owner

Looks like we could use a lockguard here, since we lock at the beginning of the method and only unlock right before returning. If you look at Thread::context, you see it being used this.

This probably applies for Thread::wakeup and Thread::raise as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
kernel/bootstrap/thread18.rb
((5 lines not shown))
93 92
   end
94 93
 
  94
+  def kill
  95
+    @dying = true
  96
+    if @sleep and @killed
  97
+      @sleep = false
  98
+      wakeup
  99
+    else
  100
+      @sleep = false
  101
+      @killed = true
  102
+      kill_prim
  103
+    end
  104
+  end
1
Dirkjan Bussink Owner

Did you try to reason about this method under concurrent scenario's? I wonder what the behavior should be when for example multiple other threads try to kill the same thread, can that for example result in the thread not being killed at all? Should we use a lock here or not?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
vm/vm.hpp
... ...
@@ -407,6 +408,7 @@
407 408
     }
408 409
 
409 410
     void register_raise(STATE, Exception* exc);
  411
+    void trigger_kill(STATE);
1
Dirkjan Bussink Owner

Curious, what's the reason here for the different naming, trigger_kill instead of register_kill?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Dirkjan Bussink
Owner

Are the comments I made here useful? We should probably merge this, I can also address the issue after merging if you want, but want to keep it going forward :).

Ryo Onodera
Collaborator

@dbussink Yes, your comments are useful. I was just working on other thread-related pull requests. If the thread raise deadlock is merged in, I want to and will merge this as a next step. :D

Dirkjan Bussink
Owner

Ok, merged the deadlock related code. Are there any changes that need to be made here before we can merge this?

Ryo Onodera
Collaborator

Working on updating this pull request.

added some commits September 03, 2012
Ryo Onodera Use LockGuards where appropriate as RAII 27e3de4
Ryo Onodera Correctly implement Thread#kill
Re-implement Thread#kill by adding new RaiseReason rather than complicating the
existing ad-hoc way of raising Exceptions.

Conceputually, Thread#kill isn't quite like exceptions. If the exception approach
wasn't replaced, it would be neccesary to really specially handle Thread::Die.

Make some failing specs pass. Also, Fix #864.
ba7667b
Ryo Onodera Implement 1.8 specific Thread#kill behavior 523f136
Dirkjan Bussink dbussink merged commit d8f1f0e into from September 27, 2012
Dirkjan Bussink dbussink closed this September 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Showing 3 unique commits by 1 author.

Sep 27, 2012
Ryo Onodera Use LockGuards where appropriate as RAII 27e3de4
Ryo Onodera Correctly implement Thread#kill
Re-implement Thread#kill by adding new RaiseReason rather than complicating the
existing ad-hoc way of raising Exceptions.

Conceputually, Thread#kill isn't quite like exceptions. If the exception approach
wasn't replaced, it would be neccesary to really specially handle Thread::Die.

Make some failing specs pass. Also, Fix #864.
ba7667b
Ryo Onodera Implement 1.8 specific Thread#kill behavior 523f136
This page is out of date. Refresh to see the latest.
17  kernel/bootstrap/thread.rb
@@ -31,6 +31,11 @@ def raise_prim(exc)
31 31
     Kernel.raise PrimitiveFailure, "Thread#raise primitive failed"
32 32
   end
33 33
 
  34
+  def kill_prim
  35
+    Rubinius.primitive :thread_kill
  36
+    Kernel.raise PrimitiveFailure, "Thread#kill primitive failed"
  37
+  end
  38
+
34 39
   def wakeup
35 40
     Rubinius.primitive :thread_wakeup
36 41
     Kernel.raise ThreadError, "Thread#wakeup primitive failed, thread may be dead"
@@ -66,8 +71,6 @@ def unlock_locks
66 71
     Kernel.raise PrimitiveFailure, "Thread#unlock_locks primitive failed"
67 72
   end
68 73
 
69  
-  class Die < Exception; end # HACK
70  
-
71 74
   @abort_on_exception = false
72 75
 
73 76
   def self.abort_on_exception
@@ -157,16 +160,6 @@ def stop?
157 160
     !alive? || @sleep
158 161
   end
159 162
 
160  
-  def kill
161  
-    @dying = true
162  
-    @sleep = false
163  
-    self.raise Die
164  
-    self
165  
-  end
166  
-
167  
-  alias_method :exit, :kill
168  
-  alias_method :terminate, :kill
169  
-
170 163
   def sleeping?
171 164
     Rubinius.synchronize(self) do
172 165
       @sleep
20  kernel/bootstrap/thread18.rb
@@ -80,8 +80,6 @@ def __run__()
80 80
           @joins.each { |join| join.send self }
81 81
         end
82 82
       end
83  
-    rescue Die
84  
-      @exception = nil
85 83
     rescue Exception => e
86 84
       # I don't really get this, but this is MRI's behavior. If we're dying
87 85
       # by request, ignore any raised exception.
@@ -109,8 +107,26 @@ def setup(prime_lock)
109 107
     @critical = false
110 108
     @dying = false
111 109
     @joins = []
  110
+    @killed = false
112 111
   end
113 112
 
  113
+  def kill
  114
+    @dying = true
  115
+    Rubinius.synchronize(self) do
  116
+      if @sleep and @killed
  117
+        @sleep = false
  118
+        wakeup
  119
+      else
  120
+        @sleep = false
  121
+        @killed = true
  122
+        kill_prim
  123
+      end
  124
+    end
  125
+  end
  126
+
  127
+  alias_method :exit, :kill
  128
+  alias_method :terminate, :kill
  129
+
114 130
   def value
115 131
     join_inner { @result }
116 132
   end
15  kernel/bootstrap/thread19.rb
@@ -65,14 +65,14 @@ def __run__()
65 65
           @joins.each { |join| join.send self }
66 66
         end
67 67
       end
68  
-    rescue Die
69  
-      @killed = true
70  
-      @exception = nil
71 68
     rescue Exception => e
72 69
       # I don't really get this, but this is MRI's behavior. If we're dying
73 70
       # by request, ignore any raised exception.
74 71
       @exception = e # unless @dying
75 72
     ensure
  73
+      if Rubinius.thread_state[0] == :thread_kill
  74
+        @killed = true
  75
+      end
76 76
       @alive = false
77 77
       Rubinius.unlock(self)
78 78
       unlock_locks
@@ -98,6 +98,15 @@ def setup(prime_lock)
98 98
     @killed = false
99 99
   end
100 100
 
  101
+  def kill
  102
+    @dying = true
  103
+    @sleep = false
  104
+    kill_prim
  105
+  end
  106
+
  107
+  alias_method :exit, :kill
  108
+  alias_method :terminate, :kill
  109
+
101 110
   def value
102 111
     join_inner do
103 112
       @killed ? nil : @result
3  spec/tags/18/ruby/core/thread/exit_tags.txt
... ...
@@ -1,3 +0,0 @@
1  
-fails:Thread#exit does not set $!
2  
-fails:Thread#exit cannot be rescued
3  
-fails:Thread#exit killing dying sleeping thread wakes up thread
3  spec/tags/18/ruby/core/thread/kill_tags.txt
... ...
@@ -1,3 +0,0 @@
1  
-fails:Thread#kill does not set $!
2  
-fails:Thread#kill cannot be rescued
3  
-fails:Thread#kill killing dying sleeping thread wakes up thread
3  spec/tags/18/ruby/core/thread/terminate_tags.txt
... ...
@@ -1,3 +0,0 @@
1  
-fails:Thread#terminate does not set $!
2  
-fails:Thread#terminate cannot be rescued
3  
-fails:Thread#terminate killing dying sleeping thread wakes up thread
2  spec/tags/19/ruby/core/thread/kill_tags.txt
... ...
@@ -1,2 +0,0 @@
1  
-fails:Thread#kill does not set $!
2  
-fails:Thread#kill cannot be rescued
2  spec/tags/19/ruby/core/thread/terminate_tags.txt
... ...
@@ -1,2 +0,0 @@
1  
-fails:Thread#terminate does not set $!
2  
-fails:Thread#terminate cannot be rescued
3  vm/builtin/system.cpp
@@ -1592,6 +1592,9 @@ namespace rubinius {
1592 1592
     case cCatchThrow:
1593 1593
       reason = state->symbol("catch_throw");
1594 1594
       break;
  1595
+    case cThreadKill:
  1596
+      reason = state->symbol("thread_kill");
  1597
+      break;
1595 1598
     default:
1596 1599
       reason = state->symbol("unknown");
1597 1600
     }
28  vm/builtin/thread.cpp
@@ -322,41 +322,57 @@ namespace rubinius {
322 322
   }
323 323
 
324 324
   Object* Thread::raise(STATE, GCToken gct, Exception* exc) {
325  
-    init_lock_.lock();
  325
+    utilities::thread::SpinLock::LockGuard lg(init_lock_);
326 326
     Thread* self = this;
327 327
     OnStack<2> os(state, self, exc);
328 328
 
329 329
     VM* vm = self->vm_;
330 330
     if(!vm) {
331  
-      self->init_lock_.unlock();
332 331
       return cNil;
333 332
     }
334 333
 
335 334
     vm->register_raise(state, exc);
336 335
 
337 336
     vm->wakeup(state, gct);
338  
-    self->init_lock_.unlock();
339 337
     return exc;
340 338
   }
341 339
 
  340
+  Object* Thread::kill(STATE, GCToken gct) {
  341
+    utilities::thread::SpinLock::LockGuard lg(init_lock_);
  342
+    Thread* self = this;
  343
+    OnStack<1> os(state, self);
  344
+
  345
+    VM* vm = self->vm_;
  346
+    if(!vm) {
  347
+      return cNil;
  348
+    }
  349
+
  350
+    if(state->vm()->thread.get() == self) {
  351
+      vm_->thread_state_.raise_thread_kill();
  352
+      return NULL;
  353
+    } else {
  354
+      vm->register_kill(state);
  355
+      vm->wakeup(state, gct);
  356
+      return self;
  357
+    }
  358
+  }
  359
+
342 360
   Object* Thread::set_priority(STATE, Fixnum* new_priority) {
343 361
     return new_priority;
344 362
   }
345 363
 
346 364
   Thread* Thread::wakeup(STATE, GCToken gct) {
347  
-    init_lock_.lock();
  365
+    utilities::thread::SpinLock::LockGuard lg(init_lock_);
348 366
     Thread* self = this;
349 367
     OnStack<1> os(state, self);
350 368
 
351 369
     VM* vm = self->vm_;
352 370
     if(alive() == cFalse || !vm) {
353  
-      self->init_lock_.unlock();
354 371
       return force_as<Thread>(Primitives::failure());
355 372
     }
356 373
 
357 374
     vm->wakeup(state, gct);
358 375
 
359  
-    self->init_lock_.unlock();
360 376
     return self;
361 377
   }
362 378
 
6  vm/builtin/thread.hpp
@@ -167,6 +167,12 @@ namespace rubinius {
167 167
     Object* raise(STATE, GCToken gct, Exception* exc);
168 168
 
169 169
     /**
  170
+     *  Kill this Thread.
  171
+     */
  172
+    // Rubinius.primitive :thread_kill
  173
+    Object* kill(STATE, GCToken gct);
  174
+
  175
+    /**
170 176
      *  Set the priority for this Thread.
171 177
      *
172 178
      *  The value is numeric, higher being more important
4  vm/instructions.cpp
@@ -161,6 +161,7 @@ Object* MachineCode::interpreter(STATE,
161 161
     // Otherwise, fall through and run the unwinds
162 162
   case cReturn:
163 163
   case cCatchThrow:
  164
+  case cThreadKill:
164 165
     // Otherwise, we're doing a long return/break unwind through
165 166
     // here. We need to run ensure blocks.
166 167
     while(current_unwind > 0) {
@@ -314,6 +315,7 @@ Object* MachineCode::uncommon_interpreter(STATE,
314 315
     // Otherwise, fall through and run the unwinds
315 316
   case cReturn:
316 317
   case cCatchThrow:
  318
+  case cThreadKill:
317 319
     // Otherwise, we're doing a long return/break unwind through
318 320
     // here. We need to run ensure blocks.
319 321
     while(current_unwind > 0) {
@@ -460,6 +462,7 @@ Object* MachineCode::debugger_interpreter(STATE,
460 462
     // Otherwise, fall through and run the unwinds
461 463
   case cReturn:
462 464
   case cCatchThrow:
  465
+  case cThreadKill:
463 466
     // Otherwise, we're doing a long return/break unwind through
464 467
     // here. We need to run ensure blocks.
465 468
     while(current_unwind > 0) {
@@ -591,6 +594,7 @@ Object* MachineCode::debugger_interpreter_continue(STATE,
591 594
     // Otherwise, fall through and run the unwinds
592 595
   case cReturn:
593 596
   case cCatchThrow:
  597
+  case cThreadKill:
594 598
     // Otherwise, we're doing a long return/break unwind through
595 599
     // here. We need to run ensure blocks.
596 600
     while(current_unwind > 0) {
3  vm/raise_reason.hpp
@@ -8,7 +8,8 @@ namespace rubinius {
8 8
     cReturn,
9 9
     cBreak,
10 10
     cExit,
11  
-    cCatchThrow
  11
+    cCatchThrow,
  12
+    cThreadKill,
12 13
   };
13 14
 }
14 15
 
5  vm/state.cpp
@@ -38,6 +38,11 @@ namespace rubinius {
38 38
       vm_->thread_state_.raise_exception(exc);
39 39
       return false;
40 40
     }
  41
+    if(vm_->interrupt_by_kill_) {
  42
+      vm_->interrupt_by_kill_ = false;
  43
+      vm_->thread_state_.raise_thread_kill();
  44
+      return false;
  45
+    }
41 46
 
42 47
     return true;
43 48
   }
4  vm/thread_state.cpp
@@ -98,4 +98,8 @@ namespace rubinius {
98 98
     raise_value_.set(value);
99 99
     throw_dest_.set(dest);
100 100
   }
  101
+
  102
+  void ThreadState::raise_thread_kill() {
  103
+    raise_reason_ = cThreadKill;
  104
+  }
101 105
 }
1  vm/thread_state.hpp
@@ -58,6 +58,7 @@ namespace rubinius {
58 58
     void raise_break(Object* value, VariableScope* dest);
59 59
     void raise_exit(Object* code);
60 60
     void raise_throw(Object* dest, Object* value);
  61
+    void raise_thread_kill();
61 62
   };
62 63
 };
63 64
 
8  vm/vm.cpp
@@ -76,6 +76,7 @@ namespace rubinius {
76 76
     , waiting_channel_(this, nil<Channel>())
77 77
     , interrupted_exception_(this, nil<Exception>())
78 78
     , interrupt_with_signal_(false)
  79
+    , interrupt_by_kill_(false)
79 80
     , waiting_header_(0)
80 81
     , custom_wakeup_(0)
81 82
     , custom_wakeup_data_(0)
@@ -428,6 +429,13 @@ namespace rubinius {
428 429
     get_attention();
429 430
   }
430 431
 
  432
+  void VM::register_kill(STATE) {
  433
+    SYNC(state);
  434
+    interrupt_by_kill_ = true;
  435
+    check_local_interrupts = true;
  436
+    get_attention();
  437
+  }
  438
+
431 439
   void VM::set_current_fiber(Fiber* fib) {
432 440
     set_stack_bounds((uintptr_t)fib->stack_start(), fib->stack_size());
433 441
     current_fiber.set(fib);
2  vm/vm.hpp
@@ -112,6 +112,7 @@ namespace rubinius {
112 112
     TypedRoot<Exception*> interrupted_exception_;
113 113
 
114 114
     bool interrupt_with_signal_;
  115
+    bool interrupt_by_kill_;
115 116
     InflatedHeader* waiting_header_;
116 117
 
117 118
     void (*custom_wakeup_)(void*);
@@ -407,6 +408,7 @@ namespace rubinius {
407 408
     }
408 409
 
409 410
     void register_raise(STATE, Exception* exc);
  411
+    void register_kill(STATE);
410 412
 
411 413
     void gc_scan(GarbageCollector* gc);
412 414
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.