search

rmagick / rmagick

Ruby bindings for ImageMagick
https://rmagick.github.io/
MIT License
696 stars 140 forks source link

CI: add back Ruby 2.5 to CI checks #339

Closed mockdeep closed 5 years ago

mockdeep commented 5 years ago

Hey @Watson1978 it looks like there's a problem with monitor on Ruby 2.5. It causes a SystemStackError. Do you have any idea why that might be?

https://travis-ci.org/rmagick/rmagick/jobs/502417921

Watson1978 commented 5 years ago

I have no idea, now. 😣I will investigate the crash...

Watson1978 commented 5 years ago

I looked the crash. it is very similar with Windows problem (https://github.com/rmagick/rmagick/pull/344).

(We might have to delete monitor method.)

Watson1978 commented 5 years ago 
monitor = proc do |mth, q, s|
  p "!" * 80
  true
end

img = Magick::Image.new(200, 200) { self.monitor = monitor }
img.resize!(20, 20)
img.monitor = nil
$ ruby -v test.rb
ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux]
"!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!"
test.rb:9: [BUG] Segmentation fault at 0x0000000000000008
ruby 2.5.3p105 (2018-10-18 revision 65156) [x86_64-linux]

-- Control frame information -----------------------------------------------
c:0003 p:---- s:0014 e:000013 CFUNC  :resize!
c:0002 p:0042 s:0008 E:0022f0 EVAL   test.rb:9 [FINISH]
c:0001 p:0000 s:0003 E:000c50 (none) [FINISH]

monitor callback just works at once. At that time it may be destroying stack in Ruby.

Ruby's checker detects broken stack at https://github.com/ruby/ruby/blob/bdd97e5f38d6ef876a1ed1206f0d7ba3e7da9983/vm_eval.c#L295 when we invoke Proc#call at https://github.com/rmagick/rmagick/blob/a156f1b165a0838c0d4cc2fa3c8c85471caf1e55/ext/RMagick/rmutil.c#L1474

Then the checker raises SEGV.

Before Ruby 2.5, looks for me that the stack checker disabled on POSIX environment. (https://github.com/ruby/ruby/blob/391f88c8431075596a6f44d8bea19d2e155e3003/gc.c#L3976-L3983)

So, this problem has occurred on Windows only before.

mockdeep commented 5 years ago

I see, so this is the same as the issue that caused us to disable monitor for Windows in #344.

mockdeep commented 5 years ago

Is there any way we can fix monitor instead of removing it?

Watson1978 commented 5 years ago

The monitor problem is so tough because it relates to Ruby internal strongly. I guess we have to create a patch to Ruby to fix the problem. If we succeed to create it, the patch might be shipped with latest Ruby....

mockdeep commented 5 years ago

@Watson1978 it sounds like you think this is a ruby bug. Is that true? Can you explain more to me why ruby_stack_check doesn't work for us here?

Watson1978 commented 5 years ago

I don't know the behavior of stack check, yet :(

Knowing things

  1. macOS environment is not affected by stack check. monitor works without any changing.
  2. When remove following two line, monitor will works on Linux.
mockdeep commented 5 years ago

One thing I notice is that there appear to be 2 errors in the test output. The first:

===============================================================================
Error: test_monitor(InfoUT)
  SystemStackError: stack level too deep
/home/travis/build/rmagick/rmagick/test/Info.rb:231:in `resize!'
/home/travis/build/rmagick/rmagick/test/Info.rb:231:in `test_monitor'
     228:       true
     229:     end
     230:     img = Magick::Image.new(2000, 2000) { self.monitor = monitor }
  => 231:     img.resize!(20, 20)
     232:     img.monitor = nil
     233:   end
     234: 
===============================================================================

And then a little later:

/home/travis/build/rmagick/rmagick/test/Preview.rb:43: [BUG] Segmentation fault at 0x00007f47167cd004
ruby 2.5.1p57 (2018-03-29 revision 63029) [x86_64-linux]
-- Control frame information -----------------------------------------------
c:0025 p:---- s:0142 e:000141 CFUNC  :preview
c:0024 p:0015 s:0137 e:000136 BLOCK  /home/travis/build/rmagick/rmagick/test/Preview.rb:43
c:0023 p:0048 s:0133 e:000132 BLOCK  /home/travis/.rvm/gems/ruby-2.5.1/gems/test-unit-2.5.5/lib/test/unit/assertions.rb:466 [FINISH]
c:0022 p:0049 s:0126 e:000125 METHOD /home/travis/.rvm/gems/ruby-2.5.1/gems/test-unit-2.5.5/lib/test/unit/assertions.rb:1442
c:0021 p:0005 s:0121 E:001008 METHOD /home/travis/.rvm/gems/ruby-2.5.1/gems/test-unit-2.5.5/lib/test/unit/assertions.rb:457
c:0020 p:0299 s:0116 E:0006e0 METHOD /home/travis/build/rmagick/rmagick/test/Preview.rb:42
...

One thing that looks surprising about this is that based on the require statements in test_all_basic.rb, it seems like Preview.rb should be running before Info.rb.

mockdeep commented 5 years ago

Woot! Passing ruby 2.5.

Watson1978 commented 5 years ago

Maybe, now we can add Ruby 2.6 to CI :)