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

Garbage collection doesn't free values released during garbage collection (via native pointers) #2253

Closed
dethredic opened this issue Mar 26, 2018 · 10 comments
Labels
feature request Requested feature memory management Related to memory management or garbage collection

Comments

@dethredic
Copy link

dethredic commented Mar 26, 2018

The following is some sample code which I hope helps demonstrates the problem.

typedef struct {
  jerry_value_t val;
} FooBar;

static void free_ctx_cb(void *native_p) {
  FooBar *ctx = (FooBar *) native_p;
  jerry_release_value(ctx->val);
  free(ctx);
}

static const jerry_object_native_info_t native_info = {
  .free_cb = free_ctx_cb,
};

jerry_value_t my_func(const jerry_value_t function_obj_p,
                      const jerry_value_t this_val,
                      const jerry_value_t argv[],
                      const jerry_length_t argc) {
  jerry_value_t obj = jerry_create_object();
  FooBar *ctx = malloc(sizeof(FooBar));
  ctx->val = jerry_acquire_value(this_val);
  jerry_set_object_native_pointer(obj, (void *) ctx, &native_info);
  return obj;
}

To finish my program I call jerry_cleanup(). This triggers garbage collection, which causes free_ctx_cb() to be run. free_ctx_cb() decrements the ref count on the variable, but the garbage collection alg doesn't seem to be aware of this new deref, so I get the following: jerry_assert_fail(assertion="JERRY_CONTEXT (jmem_heap_allocated_size) == 0")

To work around this I have to call jerry_gc(); jerry_cleanup();, but as you can imagine that only works for 1 layer. There could hypothetically be multiple more layers.

Am I using the APIs incorrectly, or should there be another loop in the garbage collection algorithm to pick up any new objects with a ref count of 0?

@LaszloLango
Copy link
Contributor

LaszloLango commented Mar 26, 2018

It looks like you are using the API correctly and jerry_cleanup should deal with it.

@martijnthe
Copy link
Contributor

Hi Phil! :)

Hmm yeah, looking the code I suspect you might have found a bug / edge case.

A native pointer free callbacks is called as part of the ecma_gc_free_object call here:
https://github.com/jerryscript-project/jerryscript/blob/master/jerry-core/ecma/base/ecma-gc.c?utf8=%E2%9C%93#L865

If in the free callback an object is released, it could affect the GC lists that are being iterated, but those lists don't seem to be updated while being iterated over.

That said, personally, I would avoid this a pattern. Here's why:
If val would have a reference to obj (i.e. someone could just add a property in JS, something along the lines of val.foo = obj) then they would never be released because of the jerry_acquire_value(this_val) call.

This new feature that @gabrielschulhof added may also help you here: #2236
You could iterate over all the objects with a native pointer (before calling jerry_cleanup) and do the cleanup then.

@zherczeg
Copy link
Member

zherczeg commented Mar 27, 2018

I think this kind of issue was discussed before. The solution would be to add a native object, similar to native pointer, which properties are marked by gc. Using its properties you can create any kind of dependency network. Or adding a callback which can be used to mark any JS values during gc time (the engine will internally ignore those which does not need to be marked). Both method creates a way to use weak references.

@martijnthe
Copy link
Contributor

Yeah, it was discussed at length over here: #1963

@gabrielschulhof
Copy link
Contributor

A naïve question: If the gc loop skips an object because there is still a reference to it, but then later on collects an object which causes via the native callback to have that reference removed, could not jerry_release_value() be made to always move the value to the end of the list so that, as the gc loop continues, it would re-encounter the object and collect it?

Another naïve question: Could we not set a flag at the beginning of the gc loop to indicate that "a gc loop is going on", and then have all objects passed into jerry_release_value() while the flag is on be added to a list which is consulted after the gc loop completes?

I guess my recent addition of jerry_objects_foreach could help with this, as long as the objects are marked in some way, like with a native pointer.

The truth is that private properties would be very useful at this point, because AFAICT JrS is getting to the point where multiple independent modules may want to attach native data to an object for different reasons. Name clashes can be addressed by also implementing symbols, and having the private properties be symbol-keyed.

As an example, V8 has both Symbol and Private, where Symbol is the ES6 Symbol type, and Private is essentially the same thing, only truly private in that it is impossible to enumerate from JS.

I actually have a rough implementation of symbols in JrS - it's highly WIP, to say the least :) Still, if there's any merit to it, I can turn that into a PR.

@zherczeg
Copy link
Member

GC uses an 1 bit mark-and-sweep model.

@akosthekiss
Copy link
Member

Pinging this issue. I see some suggestions in the discussion above to try and solve the problem. Did they help?

@rerobika
Copy link
Member

Let's revive this issue.

The problem in the code is calling API function in a native free callback is prohibited since it can cause undefined behavior. Internal properties got implemented in #3128 I think it helps to resolve this problem.

@rerobika rerobika added feature request Requested feature gc labels Oct 16, 2019
@gabrielschulhof
Copy link
Contributor

FWIW, this was discussed at TC39 tc39/proposal-weakrefs#125 and implemented in Node.js: nodejs/node#28428.

The conclusion was that, upon cleanup, the garbage collector cannot be used reliably to free resources outside the engine. So, in Node.js we retain a list of all references that were not cleaned up as part of GC (either because they are strong or because they are weak and the GC has not had a chance to free them because the environment exited first) and free the ones that have native callbacks first, followed by the ones that do not.

@rerobika
Copy link
Member

@gabrielschulhof I can agree with the conclusion, the GC is not designed for this kind of purposes. Also #3128 has landed, so IMO we can close this issue.

@dbatyai dbatyai added the memory management Related to memory management or garbage collection label Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Requested feature memory management Related to memory management or garbage collection
Projects
None yet
Development

No branches or pull requests

8 participants