Open nightpool opened 2 years ago
nightpool
commented
2 years ago The fact that this only happens the second time we create a snapshot makes me suspect some sort of GC related issue, but not sure..... EDIT: see below, this happens with GC.disable as well.
nightpool
commented
2 years ago Pretty stuck with this one, I have a dump of the snapshot if anyone feels like they'd be qualified to look at it and figure out what could have possibly gone wrong
nightpool
commented
2 years ago The other thing I noticed that makes me think this is GC related is that the first snapshot doesn't seem to leave behind any MiniRacer::Context at all, while the second snapshot definitely does (That's why I put in the ObjectSpace.to_a in the first place):
>> snapshot = MiniRacer::Snapshot.new(setup_code + webpack_js); nil
nil
>> ObjectSpace.each_object(MiniRacer::Context).to_a
=> []
>> snapshot2 = MiniRacer::Snapshot.new(setup_code + webpack_js); nil
nil
>> ObjectSpace.each_object(MiniRacer::Context).to_a
=> [#<MiniRacer::Context:0x000055a10e3028c0 @functions={}, @timeout=nil, @max_memory=nil, @current_exception=nil, @marshal_stack_depth=nil, @isolate=false, @ensure_gc_after_idle=nil, @disposed=false, @callback_mutex=#<Thread::Mutex:0x000055a10e3022a8>, @callback_running=false, @thread_raise_called=false, @eval_thread=nil>]
>>
>> fork { puts MiniRacer::Context.new(snapshot: snapshot2).eval("1 + 1") }
=> 54
/app/vendor/bundle/ruby/2.6.0/gems/mini_racer-0.6.1/lib/mini_racer.rb:178: [BUG] Segmentation fault at 0x0000000000000000EDIT: Looks like I was wrong about this, running the snippet with GC.disable still causes a crash. Back to the drawing board!
SamSaffron
commented
2 years ago What a pickle, usually when I hit bugs like this I tend to expand the testing matrix to see if I can isolate to a specific version.
On Fri, Jan 14, 2022 at 2:36 AM nightpool @.***> wrote:
EDIT: Looks like I was wrong about this, running the snippet with GC.disable still causes a crash. Back to the drawing board!
— Reply to this email directly, view it on GitHub https://github.com/rubyjs/mini_racer/issues/229#issuecomment-1012250674, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXK3GJ5WVJ6ZZY6VLYLUV3WQVANCNFSM5L2BZSRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you are subscribed to this thread.Message ID: @.***>
nightpool
commented
2 years ago Hi Sam, thanks for the reply. Since I've been reproducing the issue in our local app environment, I haven't been able to try on more recent Ruby versions. (The keyword args backwards incompatibility changes in 2.7 are really chafing). Getting a reproducible test case that works outside of our rails app is next on my list.
WRT libv8 versions, our current version is node 16.10.0, with v8 version 9.3.345.19. I've tested on node 16 latest (v8 9.4.146.24) and was able to reproduce the same issue. I've tested on node 17 latest (v8 9.6.180.15) and wasn't able to compile MiniRacer. I'm going to try node 17.1.0 next, which is the previous minor release of v8 (9.5.172.25), to see if that resolves the compilation error. Here's the compilation error I get on 9.6, if you have any idea what could be causing it:
Gem::Ext::BuildError: ERROR: Failed to build gem native extension.
current directory: /app/vendor/bundle/ruby/2.6.0/bundler/gems/mini_racer-30fd20745c56/ext/mini_racer_extension
/usr/local/bin/ruby -I /usr/local/lib/ruby/2.6.0 -r ./siteconf20220115-10087-bwtods.rb extconf.rb
checking for -lpthread... yes
creating Makefile
current directory: /app/vendor/bundle/ruby/2.6.0/bundler/gems/mini_racer-30fd20745c56/ext/mini_racer_extension
make "DESTDIR=" clean
current directory: /app/vendor/bundle/ruby/2.6.0/bundler/gems/mini_racer-30fd20745c56/ext/mini_racer_extension
make "DESTDIR="
compiling mini_racer_extension.cc
cc1plus: warning: command line option ‘-Wimplicit-int’ is valid for C/ObjC but not for C++
mini_racer_extension.cc: In function ‘VALUE convert_v8_to_ruby(v8::Isolate*, v8::Local<v8::Context>, v8::Local<v8::Value>)’:
mini_racer_extension.cc:656:34: error: invalid use of ‘v8::Name::Name’
Local<Symbol>::Cast(value)->Name());
^~~~
mini_racer_extension.cc: In function ‘void* gvl_ruby_callback(void*)’:
mini_racer_extension.cc:1288:25: warning: invalid conversion from ‘VALUE (*)(VALUE)’ {aka ‘long unsigned int (*)(long unsigned int)’} to ‘VALUE (*)(...)’ {aka ‘long unsigned int (*)(...)’} [-fpermissive]
result = rb_rescue2(protected_callback, callback_data_value,
^~~~~~~~~~~~~~~~~~
In file included from /usr/local/include/ruby-2.6.0/ruby.h:33,
from mini_racer_extension.cc:2:
/usr/local/include/ruby-2.6.0/ruby/ruby.h:1953:18: note: initializing argument 1 of ‘VALUE rb_rescue2(VALUE (*)(...), VALUE, VALUE (*)(...), VALUE, ...)’
VALUE rb_rescue2(VALUE(*)(ANYARGS),VALUE,VALUE(*)(ANYARGS),VALUE,...);
^~~~~~~~~~~~~~~~~
mini_racer_extension.cc:1289:13: warning: invalid conversion from ‘VALUE (*)(VALUE, VALUE)’ {aka ‘long unsigned int (*)(long unsigned int, long unsigned int)’} to ‘VALUE (*)(...)’ {aka ‘long unsigned int (*)(...)’} [-fpermissive]
rescue_callback, callback_data_value, rb_eException, (VALUE)0);
^~~~~~~~~~~~~~~
In file included from /usr/local/include/ruby-2.6.0/ruby.h:33,
from mini_racer_extension.cc:2:
/usr/local/include/ruby-2.6.0/ruby/ruby.h:1953:42: note: initializing argument 3 of ‘VALUE rb_rescue2(VALUE (*)(...), VALUE, VALUE (*)(...), VALUE, ...)’
VALUE rb_rescue2(VALUE(*)(ANYARGS),VALUE,VALUE(*)(ANYARGS),VALUE,...);
^~~~~~~~~~~~~~~~~
At global scope:
cc1plus: warning: unrecognized command line option ‘-Wno-self-assign’
cc1plus: warning: unrecognized command line option ‘-Wno-parentheses-equality’
cc1plus: warning: unrecognized command line option ‘-Wno-constant-logical-operand’
make: *** [Makefile:213: mini_racer_extension.o] Error 1
make failed, exit code 2I'll be honest, the rb_rescue2 errors really confuse me, since none of that code should have changed when updating the libv8 version.........
nightpool
commented
2 years ago Looks like the rb_rescue2 errors went away after I fixed the ->Name deprecation removal, so i'm able to try with node 17 now!
nightpool
commented
2 years ago Unfortunately, I'm running into the same segfault on node v17-latest:
SamSaffron
commented
2 years ago I just added a patch recently to fix rb_rescue2, does this need to be applied more widely?
https://github.com/rubyjs/mini_racer/commit/289587a087022d34447f3202cf7683afcfe3b899
Segfaulting on init is concerning, is there any process forking involved? Perhaps try single threaded platform to see if this removes the issue?
nightpool
commented
2 years ago I'm already using single threaded platform! See the test case in the OP. And yes, there is forking involved.
On Tue, Jan 18, 2022, 11:43 PM Sam @.***> wrote:
I just added a patch recently to fix rb_rescue2, does this need to be applied more widely?
289587a https://github.com/rubyjs/mini_racer/commit/289587a087022d34447f3202cf7683afcfe3b899
Segfaulting on init is concerning, is there any process forking involved? Perhaps try single threaded platform to see if this removes the issue?
— Reply to this email directly, view it on GitHub https://github.com/rubyjs/mini_racer/issues/229#issuecomment-1016104831, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABZCVYXI2EJBMAPTVZAZ4TUWZFQZANCNFSM5L2BZSRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you authored the thread.Message ID: @.***>
SamSaffron
commented
2 years ago Perhaps we should add a bit more guards internally to avoid stuff being called at the wrong time, the api is kind of silent about misuse ... if platform is already initialized we should explicitly raise an exception if you try to set it to something else.
On Wed, Jan 19, 2022 at 4:45 PM nightpool @.***> wrote:
I'm already using single threaded platform! See the test case in the OP. And yes, there is forking involved.
On Tue, Jan 18, 2022, 11:43 PM Sam @.***> wrote:
I just added a patch recently to fix rb_rescue2, does this need to be applied more widely?
289587a < https://github.com/rubyjs/mini_racer/commit/289587a087022d34447f3202cf7683afcfe3b899
Segfaulting on init is concerning, is there any process forking involved? Perhaps try single threaded platform to see if this removes the issue?
— Reply to this email directly, view it on GitHub <https://github.com/rubyjs/mini_racer/issues/229#issuecomment-1016104831 , or unsubscribe < https://github.com/notifications/unsubscribe-auth/AABZCVYXI2EJBMAPTVZAZ4TUWZFQZANCNFSM5L2BZSRA
. Triage notifications on the go with GitHub Mobile for iOS < https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
You are receiving this because you authored the thread.Message ID: @.***>
— Reply to this email directly, view it on GitHub https://github.com/rubyjs/mini_racer/issues/229#issuecomment-1016105611, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABIXJZH5X3X6FZQLGB7VTUWZFYHANCNFSM5L2BZSRA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
You are receiving this because you commented.Message ID: @.***>
nightpool
commented
2 years ago I'm not sure I understand what you mean by "the wrong time".... in my test case, I call single_threaded! before anything else, and I thought we already had the guards to prevent me from doing so here: https://github.com/rubyjs/mini_racer/blob/master/ext/mini_racer_extension/mini_racer_extension.cc#L357-L359 ?
So i'm pretty sure that i'm using SingleThreadedDefaultPlatform here......
tisba
commented
2 years ago @nightpool Do you have a stripped down version of your example? Happy to help it running through different Ruby versions and platforms.
tisba
commented
1 year ago Hey @nightpool. Are you still dealing with this issue? A couple of mini_racer versions have been released newer V8 versions as well.
Hi all! Long time no see. I'm trying to upgrade to 0.6.1 to reduce the potential for deadlocks in MiniRacer / V8 (we already use
MiniRacer::Platform.set_flags! :single_threadedon 0.4.0 / libv8-node 15.14.0.1, but notSingleThreadedDefaultPlatform). However, after upgrading to 0.6.1, I'm seeing some new segfaults when I attempt to deserialize snapshots.I can pretty reliable reproduce these issues in the console using:
However, we don't see these errors 100% of the time. It seems like after running the above code (which is a very stripped down version of starting up our Unicorn-based React app), the fork call on the last line has about an 85% chance of failing. I've attached the segfault below, the crash appears to happen in
v8::internal::Deserializer::PostProcessNewObject(v8::internal::Handle<v8::internal::Map>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::SnapshotSpace)We're using Ruby 2.6.8.