[Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Maksim Kokryashkin via Tarantool-patches]

This test could do allocation outside of the protected frame,
which could result in an uncaught OOM and test failure. This
patch adds extra `collectgarbage` calls to the test to avoid
such situations.
---
Changes in v2:
- Fixed comments as per review by Sergey Kaplun
Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-fix-flaky
PR: https://github.com/tarantool/tarantool/pull/9318
 test/tarantool-tests/lj-1004-oom-error-frame.test.lua | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
index 3be6b555..2ddb5765 100644
--- a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
+++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
@@ -10,6 +10,8 @@ test:plan(2)

 local testoomframe = require('testoomframe')

+collectgarbage()
+
 local anchor_memory = {} -- luacheck: no unused
 local function eatchunks(size)
   while true do
@@ -34,6 +36,11 @@ local function chomp()
 end

 local st, err = pcall(chomp)
+
+-- Prevent OOM outside of the protected frame.
+anchor_memory = nil
+collectgarbage()
+
 test:ok(st == false, 'on-trace error handled successfully')
 test:like(err, 'not enough memory', 'error is OOM')
 test:done(true)
--
2.39.3 (Apple Git-145)

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Sergey Kaplun via Tarantool-patches]

Hi, Maxim!
Thanks for the fixes!
LGTM!

-- 
Best regards,
Sergey Kaplun

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Sergey Bronnikov via Tarantool-patches]

Hello, Max

LGTM with a question below


On 11/9/23 16:45, Maksim Kokryashkin wrote:
> This test could do allocation outside of the protected frame,
> which could result in an uncaught OOM and test failure. This
> patch adds extra `collectgarbage` calls to the test to avoid
> such situations.
> ---
> Changes in v2:
> - Fixed comments as per review by Sergey Kaplun
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-fix-flaky
> PR: https://github.com/tarantool/tarantool/pull/9318
>   test/tarantool-tests/lj-1004-oom-error-frame.test.lua | 7 +++++++
>   1 file changed, 7 insertions(+)
>
> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
> index 3be6b555..2ddb5765 100644
> --- a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
> +++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
> @@ -10,6 +10,8 @@ test:plan(2)
>
>   local testoomframe = require('testoomframe')
>
> +collectgarbage()

Probably it is obvious, but I don't get it.

Usually for forcing garbage collecting one need call `collectgarbage()` 
two times:

for mark and sweep. You call it single time, why?


> +
>   local anchor_memory = {} -- luacheck: no unused
>   local function eatchunks(size)
>     while true do
> @@ -34,6 +36,11 @@ local function chomp()
>   end
>
>   local st, err = pcall(chomp)
> +
> +-- Prevent OOM outside of the protected frame.
> +anchor_memory = nil
> +collectgarbage()
Ditto.
> +
>   test:ok(st == false, 'on-trace error handled successfully')
>   test:like(err, 'not enough memory', 'error is OOM')
>   test:done(true)
> --
> 2.39.3 (Apple Git-145)
>

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Sergey Bronnikov via Tarantool-patches]

Just for the record:

Fixed test executed 500 times without fails, test with reverted fix had 
failed on 138 iteration with:

/home/sergeyb/sources/MRG/tarantool/third_party/luajit/test/tarantool-tests/lj-1004-oom-error-frame.test.lua 
(Wstat: 256 Tests: 0 Failed: 0)
   Non-zero exit status: 1
   Parse errors: Bad plan.  You planned 2 tests but ran 0.
Files=101, Tests=480,  4 wallclock secs ( 0.25 usr  0.11 sys + 8.70 
cusr  4.55 csys = 13.61 CPU)
Result: FAIL

On 11/18/23 19:52, Sergey Bronnikov via Tarantool-patches wrote:

<snipped>

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Maxim Kokryashkin via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 2369 bytes --]


Hi!
Thanks for the review!
Here is the answer from the «Programming in Lua» (3rd edition), paragraph 32.2:
> From Lua we use the collectgarbage function: collectgarbage(what [, data])
> Both offer the same functionality. The what argument (an enumeration value in C, a string in Lua) specifies what to do. The > options are:
<snipped>
> LUA_GCCOLLECT (“collect”): performs a complete garbage-collection cycle, so that all unreachable objects are collected and finalized. This is the default option for collectgarbage.
 
--
Best regards,
Maxim Kokryashkin
 
  
>Суббота, 18 ноября 2023, 19:52 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>Hello, Max
>
>LGTM with a question below
>
>
>On 11/9/23 16:45, Maksim Kokryashkin wrote:
>> This test could do allocation outside of the protected frame,
>> which could result in an uncaught OOM and test failure. This
>> patch adds extra `collectgarbage` calls to the test to avoid
>> such situations.
>> ---
>> Changes in v2:
>> - Fixed comments as per review by Sergey Kaplun
>> Branch:  https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-fix-flaky
>> PR:  https://github.com/tarantool/tarantool/pull/9318
>> test/tarantool-tests/lj-1004-oom-error-frame.test.lua | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
>> index 3be6b555..2ddb5765 100644
>> --- a/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
>> +++ b/test/tarantool-tests/lj-1004-oom-error-frame.test.lua
>> @@ -10,6 +10,8 @@ test:plan(2)
>>
>> local testoomframe = require('testoomframe')
>>
>> +collectgarbage()
>
>Probably it is obvious, but I don't get it.
>
>Usually for forcing garbage collecting one need call `collectgarbage()`
>two times:
>
>for mark and sweep. You call it single time, why?
>
>
>> +
>> local anchor_memory = {} -- luacheck: no unused
>> local function eatchunks(size)
>> while true do
>> @@ -34,6 +36,11 @@ local function chomp()
>> end
>>
>> local st, err = pcall(chomp)
>> +
>> +-- Prevent OOM outside of the protected frame.
>> +anchor_memory = nil
>> +collectgarbage()
>Ditto.
>> +
>> test:ok(st == false, 'on-trace error handled successfully')
>> test:like(err, 'not enough memory', 'error is OOM')
>> test:done(true)
>> --
>> 2.39.3 (Apple Git-145)
>>
 

[-- Attachment #2: Type: text/html, Size: 3300 bytes --]

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Igor Munkin via Tarantool-patches]

Sergey,

On 18.11.23, Sergey Bronnikov via Tarantool-patches wrote:
> Hello, Max
> 
> LGTM with a question below
> 
> 
> On 11/9/23 16:45, Maksim Kokryashkin wrote:

<snipped>

> > 
> > +collectgarbage()
> 
> Probably it is obvious, but I don't get it.
> 
> Usually for forcing garbage collecting one need call `collectgarbage()` two
> times:
> 
> for mark and sweep. You call it single time, why?

The reason of two consequent <collectgarbage> calls is not for "mark and
sweep" but rather for "full GC cycle + finalization of the resurrected
objects". The first call implements full (even 1.5) GC cycle; as a
result of this call some *objects* can be *released*, but the
corresponding *memory* is preserved in another list (gc->mmudata, IIRC)
to properly finalize the object (i.e. call __gc metamethod). Hence, the
second call finally releases the memory for the objects resurrected
within the sweep phase of the first call.

Since Max allocates plain FFI objects with no <ffi.gc> finalizers,
there is no need in the second <collectgarbage> call here.

> 

<snipped>

> > 

-- 
Best regards,
IM

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Sergey Bronnikov via Tarantool-patches]

Igor,


thanks for explanation!

On 11/20/23 17:53, Igor Munkin wrote:
> Sergey,
>
> On 18.11.23, Sergey Bronnikov via Tarantool-patches wrote:
>> Hello, Max
>>
>> LGTM with a question below
>>
>>
>> On 11/9/23 16:45, Maksim Kokryashkin wrote:
> <snipped>
>
>>> +collectgarbage()
>> Probably it is obvious, but I don't get it.
>>
>> Usually for forcing garbage collecting one need call `collectgarbage()` two
>> times:
>>
>> for mark and sweep. You call it single time, why?
> The reason of two consequent <collectgarbage> calls is not for "mark and
> sweep" but rather for "full GC cycle + finalization of the resurrected
> objects". The first call implements full (even 1.5) GC cycle; as a
> result of this call some *objects* can be *released*, but the
> corresponding *memory* is preserved in another list (gc->mmudata, IIRC)
> to properly finalize the object (i.e. call __gc metamethod). Hence, the
> second call finally releases the memory for the objects resurrected
> within the sweep phase of the first call.
>
> Since Max allocates plain FFI objects with no <ffi.gc> finalizers,
> there is no need in the second <collectgarbage> call here.
>
> <snipped>
>

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Sergey Bronnikov via Tarantool-patches]

[-- Attachment #1: Type: text/plain, Size: 812 bytes --]

Max,

I would put this text to a comment for clarity.

Feel free to ignore.

On 11/20/23 17:53, Igor Munkin wrote:

<snipped>
> The reason of two consequent <collectgarbage> calls is not for "mark and
> sweep" but rather for "full GC cycle + finalization of the resurrected
> objects". The first call implements full (even 1.5) GC cycle; as a
> result of this call some *objects* can be *released*, but the
> corresponding *memory* is preserved in another list (gc->mmudata, IIRC)
> to properly finalize the object (i.e. call __gc metamethod). Hence, the
> second call finally releases the memory for the objects resurrected
> within the sweep phase of the first call.
>
> Since Max allocates plain FFI objects with no <ffi.gc> finalizers,
> there is no need in the second <collectgarbage> call here.
>
<snipped>

[-- Attachment #2: Type: text/html, Size: 1334 bytes --]

Re: [Tarantool-patches] [PATCH luajit v2] test: fix flaky OOM error frame test

[Igor Munkin via Tarantool-patches]

Max,

I've checked this patch into tarantool/master branch in tarantool/luajit
and bumped a new version in master.

On 09.11.23, Maksim Kokryashkin via Tarantool-patches wrote:
> This test could do allocation outside of the protected frame,
> which could result in an uncaught OOM and test failure. This
> patch adds extra `collectgarbage` calls to the test to avoid
> such situations.
> ---
> Changes in v2:
> - Fixed comments as per review by Sergey Kaplun
> Branch: https://github.com/tarantool/luajit/tree/fckxorg/lj-1004-fix-flaky
> PR: https://github.com/tarantool/tarantool/pull/9318
>  test/tarantool-tests/lj-1004-oom-error-frame.test.lua | 7 +++++++
>  1 file changed, 7 insertions(+)
> 

<snipped>

> --
> 2.39.3 (Apple Git-145)
> 

-- 
Best regards,
IM