From 4f40f41ce48716541b1ea85ad765dc8908d7da12 Mon Sep 17 00:00:00 2001 From: Sam Saffron Date: Fri, 15 May 2020 13:36:43 +1000 Subject: [PATCH] FIX: to keep api consistent ensure_gc_after_idle should be in ms Also introduce strict param checking for all params passed to context This avoids typo errors being totally hidden from user --- CHANGELOG | 8 +++++++ README.md | 2 +- lib/mini_racer.rb | 44 ++++++++++++++++++++++++++------------- lib/mini_racer/version.rb | 2 +- test/mini_racer_test.rb | 13 +++++++++--- 5 files changed, 49 insertions(+), 20 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index ae11ca9c..8ab98aff 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,6 +1,14 @@ - 15-05-2020 +- 0.2.14 + + - FIX: ensure_gc_after_idle should take in milliseconds like the rest of the APIs not seconds + - FEATURE: strict params on MiniRacer::Context.new + +- 15-05-2020 + - 0.2.13 + - FIX: edge case around ensure_gc_after_idle possibly firing when context is not idle - 15-05-2020 diff --git a/README.md b/README.md index 52fca9db..72481125 100644 --- a/README.md +++ b/README.md @@ -239,7 +239,7 @@ This can come in handy to force V8 GC runs for example in between requests if yo Note that this method maps directly to [`v8::Isolate::IdleNotification`](http://bespin.cz/~ondras/html/classv8_1_1Isolate.html#aea16cbb2e351de9a3ae7be2b7cb48297), and that in particular its return value is the same (true if there is no further garbage to collect, false otherwise) and the same caveats apply, in particular that `there is no guarantee that the [call will return] within the time limit.` -Additionally you may automate this process on a context by defining it with `MiniRacer::Content.new(ensure_gc_after_idle: 1)`. Using this will ensure V8 will run a full GC using `context.isolate.low_memory_notification` 1 second after the last eval on the context. Low memory notification is both slower and more aggressive than an idle_notification and will ensure long living isolates use minimal amounts of memory. +Additionally you may automate this process on a context by defining it with `MiniRacer::Content.new(ensure_gc_after_idle: 1000)`. Using this will ensure V8 will run a full GC using `context.isolate.low_memory_notification` 1 second after the last eval on the context. Low memory notification is both slower and more aggressive than an idle_notification and will ensure long living isolates use minimal amounts of memory. ### V8 Runtime flags diff --git a/lib/mini_racer.rb b/lib/mini_racer.rb index c8e0f400..2e62e469 100644 --- a/lib/mini_racer.rb +++ b/lib/mini_racer.rb @@ -130,23 +130,22 @@ def initialize(name, callback, parent) end end - def initialize(options = nil) + def initialize(max_memory: nil, timeout: nil, isolate: nil, ensure_gc_after_idle: nil, snapshot: nil) options ||= {} - check_init_options!(options) + check_init_options!(isolate: isolate, snapshot: snapshot, max_memory: max_memory, ensure_gc_after_idle: ensure_gc_after_idle, timeout: timeout) @functions = {} @timeout = nil @max_memory = nil @current_exception = nil - @timeout = options[:timeout] - if options[:max_memory].is_a?(Numeric) && options[:max_memory] > 0 - @max_memory = options[:max_memory] - end + @timeout = timeout + @max_memory = max_memory + # false signals it should be fetched if requested - @isolate = options[:isolate] || false + @isolate = isolate || false - @ensure_gc_after_idle = options[:ensure_gc_after_idle] + @ensure_gc_after_idle = ensure_gc_after_idle if @ensure_gc_after_idle @last_eval = nil @@ -162,7 +161,7 @@ def initialize(options = nil) @eval_thread = nil # defined in the C class - init_unsafe(options[:isolate], options[:snapshot]) + init_unsafe(isolate, snapshot) end def isolate @@ -290,6 +289,7 @@ def ensure_gc_thread @ensure_gc_mutex.synchronize do @ensure_gc_thread = nil if !@ensure_gc_thread&.alive? @ensure_gc_thread ||= Thread.new do + ensure_gc_after_idle_seconds = @ensure_gc_after_idle / 1000.0 done = false while !done now = Process.clock_gettime(Process::CLOCK_MONOTONIC) @@ -299,7 +299,7 @@ def ensure_gc_thread break end - if !@eval_thread && @ensure_gc_after_idle < now - @last_eval + if !@eval_thread && ensure_gc_after_idle_seconds < now - @last_eval @ensure_gc_mutex.synchronize do isolate_mutex.synchronize do if !@eval_thread @@ -310,7 +310,7 @@ def ensure_gc_thread end end end - sleep @ensure_gc_after_idle if !done + sleep ensure_gc_after_idle_seconds if !done end end end @@ -369,15 +369,29 @@ def timeout(&blk) rp.close if rp end - def check_init_options!(options) - assert_option_is_nil_or_a('isolate', options[:isolate], Isolate) - assert_option_is_nil_or_a('snapshot', options[:snapshot], Snapshot) + def check_init_options!(isolate:, snapshot:, max_memory:, ensure_gc_after_idle:, timeout:) + assert_option_is_nil_or_a('isolate', isolate, Isolate) + assert_option_is_nil_or_a('snapshot', snapshot, Snapshot) - if options[:isolate] && options[:snapshot] + assert_numeric_or_nil('max_memory', max_memory, min_value: 10_000) + assert_numeric_or_nil('ensure_gc_after_idle', ensure_gc_after_idle, min_value: 1) + assert_numeric_or_nil('timeout', timeout, min_value: 1) + + if isolate && snapshot raise ArgumentError, 'can only pass one of isolate and snapshot options' end end + def assert_numeric_or_nil(option_name, object, min_value:) + if object.is_a?(Numeric) && object < min_value + raise ArgumentError, "#{option_name} must be larger than #{min_value}" + end + + if !object.nil? && !object.is_a?(Numeric) + raise ArgumentError, "#{option_name} must be a number, passed a #{object.inspect}" + end + end + def assert_option_is_nil_or_a(option_name, object, klass) unless object.nil? || object.is_a?(klass) raise ArgumentError, "#{option_name} must be a #{klass} object, passed a #{object.inspect}" diff --git a/lib/mini_racer/version.rb b/lib/mini_racer/version.rb index d8d93451..b4b9695c 100644 --- a/lib/mini_racer/version.rb +++ b/lib/mini_racer/version.rb @@ -1,5 +1,5 @@ # frozen_string_literal: true module MiniRacer - VERSION = "0.2.13" + VERSION = "0.2.14" end diff --git a/test/mini_racer_test.rb b/test/mini_racer_test.rb index 3c82ae6c..ff269431 100644 --- a/test/mini_racer_test.rb +++ b/test/mini_racer_test.rb @@ -326,8 +326,9 @@ def test_max_memory_for_call end def test_negative_max_memory - context = MiniRacer::Context.new(max_memory: -200_000_000) - assert_nil(context.instance_variable_get(:@max_memory)) + assert_raises(ArgumentError) do + MiniRacer::Context.new(max_memory: -200_000_000) + end end module Echo @@ -722,8 +723,14 @@ def test_releasing_memory assert((end_heap - start_heap).abs < 1000, "expecting most of the 1_000_000 long string to be freed") end + def test_bad_params + assert_raises do + MiniRacer::Context.new(random: :thing) + end + end + def test_ensure_gc - context = MiniRacer::Context.new(ensure_gc_after_idle: 0.001) + context = MiniRacer::Context.new(ensure_gc_after_idle: 1) context.isolate.low_memory_notification start_heap = context.heap_stats[:used_heap_size]