Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gc_possible_root: Assertion `(ref)->gc.u.v.type == 7 || (ref)->gc.u.v.type == 8' failed. #32

Closed
taowen opened this issue Oct 28, 2016 · 3 comments
Assignees

Comments

@taowen
Copy link

taowen commented Oct 28, 2016

package php

import (
    "testing"
    "github.com/stretchr/testify/assert"
    "github.com/deuill/go-php/engine"
)

func Test_map_value(t *testing.T) {
    should := assert.New(t)
    theEngine, err := engine.New()
    should.Nil(err)
    defer theEngine.Destroy()
    context, err := theEngine.NewContext()
    should.Nil(err)
    defer context.Destroy()
    context.Bind("hello", map[string]interface{}{})
}
    for _, v := range c.values {
        v.Destroy()
    }
    c.values = nil

    C.context_destroy(c.context)
    c.context = nil

when the values destroyed, then the context destroy will trigger gc_possible_root assertion error. Found in php-7.0.12

Complete error message

php.test: /home/xiaoju/disf-php/php-src/php-7.0.12/Zend/zend_gc.c:226: gc_possible_root: Assertion `(ref)->gc.u.v.type == 7 || (ref)->gc.u.v.type == 8' failed.
SIGABRT: abort
PC=0x7f051472e428 m=0
signal arrived during cgo execution
@taowen
Copy link
Author

taowen commented Oct 28, 2016

call zend_hash_str_del(&EG(symbol_table), name, strlen(name)); to remove bind name from symbol table seems like fixed the problem. Isn't the value destroy too brute force?

@deuill
Copy link
Owner

deuill commented Oct 29, 2016

Possibly so, I'll need to delve into this a bit more since the semantics between what gets de-allocated when destroying values and contexts are still unclear to me... The current implementation does a zval_dtor, which seems to be the right thing to do, but maybe not.

@deuill deuill self-assigned this Oct 29, 2016
@taowen
Copy link
Author

taowen commented Nov 4, 2016

I have found a fix for this problem. It is in my branch: https://github.com/taowen/go-php/blob/memory-leak-fix/engine/value.go

Problem 1, nested data structure memory leak

Array/Map works like a container owning its element. So when add zval to it, and it can keep track of its lifetime. However object property assignment is a copy of value, so after assign it, we need to free the zval created before, https://github.com/taowen/go-php/blob/memory-leak-fix/engine/value.go#L139

Problem 2, context.bind cause gc error

When a zval is added to symbol table. We no longer need to call zval_dtor. The zend vm will tear down the symbol table properly after shutdown. So, we just skip the destroying the value. Think context.bind as passing the memory ownership.

Problem 3, engine_value itself lifecycle

Because the engine_value itself is heap allocated. value_destroy will destroy the zval and engine_value wrapper heap memory. Sometime, we just want to destroy the wrapper, leaving the zval managed by the zend vm. It is too hard to keep track of two piece of memory (just like why zval instead zval* from php5 to php7). I just dropped the wrapper all together, using zval directly.

Now, I think all memory problem goes away. I am heading to create a php7 only version of this. It is just too much work to support both php5 and php7.

@deuill deuill closed this as completed Jun 30, 2017
borancar pushed a commit to borancar/go-php that referenced this issue Dec 1, 2019
feature/metrickeys - Updating telegraf to send metrics to eventhubs

Approved-by: Boran Car <boran.car@antstream.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants