lloeki
commented
3 months ago Using it would therefore make low_memory_notification useless while the isolate is not in use, which appears to be an intended use-case for the method.
Correct. The original idea behind this is to repeatedly run short-lived scripts and get results, but they might accumulate garbage. In between, when memory usage gets too high, trigger libv8 GC via low mem notification.
In that specific use case this fits the bill:
In particular, doing so would cause any rb-thread calling low_memory_notification to block until other threads using the isolate have unlocked it (for JavaScript execution, this means the script has finished running).
So it seems like in that case, which is the only one where mini_racer makes use of low_memory_notification, the following would be fine:
Normally I would simply slap a
Locker guard { isolate_info->isolate }prior to the call
I guess the following bit of code (notably the @eval_thread check) was supposed to sort of ensure that on the Ruby side but it's not sufficient anymore and doesn't cater for whatever libv8 is doing internally: https://github.com/rubyjs/mini_racer/blob/b7fd92e60b2b8643a017543a90f75d0398cbb1a8/lib/mini_racer.rb#L321-L322
The two debug+release cases you described plus the lack of any other feature that would fit the bill seem to make it clear that v8 intents us to just use the lock guard, so let's do that.
tisba
Background
V8 uses a locking system (via
v8::Locker) to ensure Isolates can only be used from one thread at a time. A lot of methods (the headers define it as "every method below this notice") have an API contract requiring that the Isolate is locked by the current thread before they are called. This includesIsolate::LowMemoryNotification.The Problem
mini_racer does not lock the isolate prior to calling
LowMemoryNotification. https://github.com/rubyjs/mini_racer/blob/b7fd92e60b2b8643a017543a90f75d0398cbb1a8/ext/mini_racer_extension/mini_racer_extension.cc#L962-L970With a debug build of V8, this crashes the process when exiting the garbage collector, since it (correctly) detects that the current thread is not the owning thread of the V8 heap.
With a release build, this may cause data races, particularly if one thread is running JavaScript code while another thread calls
low_memory_notificationon that same isolateSolutions
Normally I would simply slap a
Locker guard { isolate_info->isolate }prior to the call and open a PR, but I'm not sure that is the correct solution here.In particular, doing so would cause any rb-thread calling
low_memory_notificationto block until other threads using the isolate have unlocked it (for JavaScript execution, this means the script has finished running).Unfortunately V8 does not provide a
try_lock-esque API that allows taking an Isolate lock without blocking.There is a method for interrupting running script execution (
Isolate::RequestInterrupt), but callbacks registered only fire during script execution. Using it would therefore makelow_memory_notificationuseless while the isolate is not in use, which appears to be an intended use-case for the method.There is an
Isolate::IsInUsemethod, which would theoretically permit checking if another thread is currently running JavaScript or performing some other operation on the isolate; however this method is also within the "requires locking" set.In addition, using it without locking might result in its own data race, where the thread calling
low_memory_notificationchecksIsInUse, receivesfalseand is then interrupted by a thread starting long-running JavaScript execution. The LMN-thread would try to acquire a lock on the Isolate and block, as per the simple solution.(discovered while digging through #283's problems)