New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread.current.status is wrong for killed thread #4705

Closed
iaddict opened this Issue Jul 3, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@iaddict
Contributor

iaddict commented Jul 3, 2017

Environment

Provide at least:

  • JRuby version: jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 Java HotSpot(TM) 64-Bit Server VM 25.112-b16 on 1.8.0_112-b16 [darwin-x86_64]
  • Operating system and platform: Darwin de01pc11303.global.jhcn.net 16.6.0 Darwin Kernel Version 16.6.0: Fri Apr 14 16:21:16 PDT 2017; root:xnu-3789.60.24~6/RELEASE_X86_64 x86_64

Expected Behavior

A sleeping thread that gets killed should have status 'aborting' and not 'run'.

Actual Behavior

The following code outputs 'run' on jruby, whereas on mri it outputs 'aborting'.

t = Thread.new {
  begin
    sleep
  ensure
    puts Thread.current.status # should output 'aborting' when killed
  end
}
t.kill

Example run on jruby:

irb(main):020:0> t = Thread.new { begin; sleep; ensure; puts Thread.current.status; end }
=> #<Thread:0x79ad8b2f@(irb):20 sleep>
irb(main):021:0> t.kill
run
=> #<Thread:0x79ad8b2f@(irb):20 dead>

Example run on mri ruby:

irb(main):013:0>  t = Thread.new { begin; sleep; ensure; puts Thread.current.status; end }
=> #<Thread:0x007faefc8bad78@(irb):13 run>
irb(main):014:0> t.kill
aborting
=> #<Thread:0x007faefc8bad78@(irb):13 sleep>

This code is a simplified version of ActiveRecord transaction code that fails.
See:

@iaddict iaddict referenced this issue Jul 3, 2017

Open

Jdbc mode #599

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Jul 3, 2017

Member

did not expect using Thread.current.status to manage transaction state. haven't seen this anywhere else, anyone nows if this is really necessary or just a bad design decision ?

Member

kares commented Jul 3, 2017

did not expect using Thread.current.status to manage transaction state. haven't seen this anywhere else, anyone nows if this is really necessary or just a bad design decision ?

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Jul 3, 2017

Member

Interesting, I recently looked at this in TruffleRuby and could not figure how the user can observe the aborting status.
We should definitely add a spec for this.
Note that you might need a sleep 0.1 before the t.kill in the example above to observe it, as otherwise it could kill the thread before it even reaches the sleep.

Member

eregon commented Jul 3, 2017

Interesting, I recently looked at this in TruffleRuby and could not figure how the user can observe the aborting status.
We should definitely add a spec for this.
Note that you might need a sleep 0.1 before the t.kill in the example above to observe it, as otherwise it could kill the thread before it even reaches the sleep.

@iaddict

This comment has been minimized.

Show comment
Hide comment
@iaddict

iaddict Jul 3, 2017

Contributor

Here is a slightly better version of the sample code which uses a Queue to make sure that the thread is sleeping.

q = Queue.new
t = Thread.new { begin; q.push nil; sleep; ensure; puts Thread.current.status; end }
q.pop
t.kill
Contributor

iaddict commented Jul 3, 2017

Here is a slightly better version of the sample code which uses a Queue to make sure that the thread is sleeping.

q = Queue.new
t = Thread.new { begin; q.push nil; sleep; ensure; puts Thread.current.status; end }
q.pop
t.kill

iaddict added a commit to iaddict/spec that referenced this issue Jul 4, 2017

Test status of dying threads
The status of a dying running or sleeping thread should be 'aborting'.
On jruby this is not the case. These spec additions show this issue.

There already is an open issue for jruby: jruby/jruby#4705

```
$ ruby ./jruby/mspec/bin/mspec-run core/thread/stop_spec.rb
jruby 9.1.12.0 (2.3.3) 2017-06-15 33c6439 Java HotSpot(TM) 64-Bit Server VM 25.112-b16 on 1.8.0_112-b16 +jit [darwin-x86_64]
                                                                                             
1)
Thread#status describes a dying running thread FAILED
Expected "run"
 to equal "aborting"

./jruby/ruby-spec/core/thread/stop_spec.rb:60:in `block in (root)'
org/jruby/RubyBasicObject.java:1691:in `instance_eval'
org/jruby/RubyEnumerable.java:1596:in `all?'
org/jruby/RubyFixnum.java:299:in `times'
org/jruby/RubyArray.java:1734:in `each'
./jruby/ruby-spec/core/thread/stop_spec.rb:58:in `<main>'
org/jruby/RubyKernel.java:979:in `load'
org/jruby/RubyBasicObject.java:1691:in `instance_eval'
org/jruby/RubyArray.java:1734:in `each'
                                                                                             
2)
Thread#status describes a dying sleeping thread FAILED
Expected "run"
 to equal "aborting"

./jruby/ruby-spec/core/thread/stop_spec.rb:64:in `block in (root)'
org/jruby/RubyBasicObject.java:1691:in `instance_eval'
org/jruby/RubyEnumerable.java:1596:in `all?'
org/jruby/RubyFixnum.java:299:in `times'
org/jruby/RubyArray.java:1734:in `each'
./jruby/ruby-spec/core/thread/stop_spec.rb:58:in `<main>'
org/jruby/RubyKernel.java:979:in `load'
org/jruby/RubyBasicObject.java:1691:in `instance_eval'
org/jruby/RubyArray.java:1734:in `each'
[| | ==================100%================== | 00:00:00]      2F      0E 

Finished in 0.175762 seconds

1 file, 12 examples, 13 expectations, 2 failures, 0 errors, 0 tagged
```
@iaddict

This comment has been minimized.

Show comment
Hide comment
@iaddict

iaddict Sep 11, 2017

Contributor

I ran the specs again with the current jruby master branch and they did pass, albeit the above referenced pull request did not catch the issue in its entirety. I created another pull request that properly tests the status of a sleeping killed thread that fails on jruby but passes on mri. See: ruby/spec#462. Thus the problem is still present.

Contributor

iaddict commented Sep 11, 2017

I ran the specs again with the current jruby master branch and they did pass, albeit the above referenced pull request did not catch the issue in its entirety. I created another pull request that properly tests the status of a sleeping killed thread that fails on jruby but passes on mri. See: ruby/spec#462. Thus the problem is still present.

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Sep 14, 2017

Member

That spec is now in jruby and fails as expected:
https://travis-ci.org/jruby/jruby/jobs/275547991

     [exec] Thread#status reports aborting on an externally killed thread that sleeps FAILED
     [exec] Expected "run"
     [exec]  to equal "aborting"
     [exec] 
     [exec] /home/travis/build/jruby/jruby/spec/ruby/core/thread/status_spec.rb:58:in `block in (root)'
     [exec] org/jruby/RubyBasicObject.java:1750:in `instance_eval'
     [exec] org/jruby/RubyEnumerable.java:1704:in `all?'
     [exec] org/jruby/RubyArray.java:1764:in `each'
     [exec] /home/travis/build/jruby/jruby/spec/ruby/core/thread/status_spec.rb:4:in `<main>'
     [exec] org/jruby/RubyKernel.java:986:in `load'
     [exec] org/jruby/RubyBasicObject.java:1750:in `instance_eval'
     [exec] org/jruby/RubyArray.java:1764:in `each'
Member

eregon commented Sep 14, 2017

That spec is now in jruby and fails as expected:
https://travis-ci.org/jruby/jruby/jobs/275547991

     [exec] Thread#status reports aborting on an externally killed thread that sleeps FAILED
     [exec] Expected "run"
     [exec]  to equal "aborting"
     [exec] 
     [exec] /home/travis/build/jruby/jruby/spec/ruby/core/thread/status_spec.rb:58:in `block in (root)'
     [exec] org/jruby/RubyBasicObject.java:1750:in `instance_eval'
     [exec] org/jruby/RubyEnumerable.java:1704:in `all?'
     [exec] org/jruby/RubyArray.java:1764:in `each'
     [exec] /home/travis/build/jruby/jruby/spec/ruby/core/thread/status_spec.rb:4:in `<main>'
     [exec] org/jruby/RubyKernel.java:986:in `load'
     [exec] org/jruby/RubyBasicObject.java:1750:in `instance_eval'
     [exec] org/jruby/RubyArray.java:1764:in `each'
@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Oct 5, 2017

Member

This is still a bug. I tagged the spec so the CI is green in 73b0114

Member

eregon commented Oct 5, 2017

This is still a bug. I tagged the spec so the CI is green in 73b0114

@iaddict

This comment has been minimized.

Show comment
Hide comment
@iaddict

iaddict Oct 5, 2017

Contributor

@eregon Does this mean the bug does not get fixed anytime soon? The AR and transactions (mentioned above) will thus persist?

Contributor

iaddict commented Oct 5, 2017

@eregon Does this mean the bug does not get fixed anytime soon? The AR and transactions (mentioned above) will thus persist?

@eregon

This comment has been minimized.

Show comment
Hide comment
@eregon

eregon Oct 5, 2017

Member

I'm not fixing the bug in JRuby, merely updating the specs.
But I guess @headius might take care of it.

Member

eregon commented Oct 5, 2017

I'm not fixing the bug in JRuby, merely updating the specs.
But I guess @headius might take care of it.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Nov 8, 2017

Member

Additional commit for this, to actually set aborting status upon kill: 919832e

Member

headius commented Nov 8, 2017

Additional commit for this, to actually set aborting status upon kill: 919832e

@headius headius added this to the JRuby 9.1.14.0 milestone Nov 8, 2017

@headius headius added the core label Nov 8, 2017

@headius headius closed this Nov 8, 2017

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