8000 py: Clear finalizer flag when calling gc_free. by dhylands · Pull Request #1595 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

py: Clear finalizer flag when calling gc_free. #1595

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

Closed
wants to merge 1 commit into from

Conversation

dhylands
Copy link
Contributor
@dhylands dhylands commented Nov 6, 2015

Currently, the only place that clears the bit is in gc_collect.
So if a block with a finalizer is allocated, and subsequently
freed, and then the block is reallocated with no finalizer then
the bit remains set.

This could also be fixed by having gc_alloc clear the bit, but
I'm pretty sure that free is called way less than alloc, so doing
it in free is more efficient.

Currently, the only place that clears the bit is in gc_collect.
So if a block with a finalizer is allocated, and subsequently
freed, and then the block is reallocated with no finalizer then
the bit remains set.

This could also be fixed by having gc_alloc clear the bit, but
I'm pretty sure that free is called way less than alloc, so doing
it in free is more efficient.
@dpgeorge
Copy link
Member
dpgeorge commented Nov 6, 2015

Very nice work!

But shouldn't in this case the finaliser be called? (ie when finalisable memory is explicitly free'd)

@dhylands
Copy link
Contributor Author
dhylands commented Nov 6, 2015

It seems to to only call the finalizer in gc_sweep, and gc_free doesn't call the finalizer.

I'm not sure what the intent was. I think we need to review gc_realloc as well. It looks like it copies the finalizer bit to the newly allocated block, and then calls gc_free.

There seem to be 2 ways that memory can be freed:

1 - through an explicit call to something like m_del_obj, or free, or gc_free
2 - through gc_sweep doing a gc.

The only place that I could find that calls m_new_obj_with_finaliser and explicitly frees it is in stmhal/file.c:
https://github.com/micropython/micropython/blob/master/stmhal/file.c#L192-L198

that code would have a different bug if the finalizer was called due to the call to m_del_obj (since o->fp would be uninitlialized and f_close would then get called on that uninitialized object).

@dpgeorge
Copy link
Member
dpgeorge commented Nov 6, 2015

Ok, you have definitely found the cause of the bug. You are right we need to decide on whether gc_free calls a finaliser (I think it should, but it could be that it doesn't and it's up to the caller to know that it must explicitly call the finaliser, since it's anyway explicitly calling free).

@dhylands
Copy link
Contributor Author
dhylands commented Nov 6, 2015

If we were to decide that gc_free should call a finalizer, then we'd need to fix gc_realloc to arrange things such that we don't call the finalizer on the "old" block that it frees.

The current behaviour actually mirrors C++'s use of destructors. The destructor will only be called on completely constructed objects. So if an exception is thrown during a constructor, the matching destructor of the object being constructed doesn't get called (any constructed members will be destructed - hows that for confusing :)

@pfalcon
Copy link
Contributor
pfalcon commented Nov 7, 2015

My initial motion was to say that gc_free() not calling finalizer was a bug (oversight). So, thanks for analysis, @dhylands , it seems it shouldn't - at least for now. Generally, we should be very careful with GC/memalloc code, and not try to "over-fix" - any change can have not easily predictable consequences. So, let's fix issue at hand and move on. Most what gc_free deserves is comment like "TODO: doesn't call finalizer".

@danicampora
Copy link
Member

it's up to the caller to know that it must explicitly call the finaliser, since it's anyway explicitly calling free

I think this makes sense.

@dpgeorge
Copy link
Member
dpgeorge commented Nov 7, 2015

Generally, we should be very careful with GC/memalloc code, and not try to "over-fix" - any change can have not easily predictable consequences. So, let's fix issue at hand and move on. Most what gc_free deserves is comment like "TODO: doesn't call finalizer".

+1

So, I'll take @dhylands' fix and add that comment.

@dpgeorge
Copy link
Member
dpgeorge commented Nov 7, 2015

Merged in 7f3c0d1.

@dpgeorge dpgeorge closed this Nov 7, 2015
@dhylands dhylands deleted the gc-finializer-free branch November 8, 2015 02:39
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

Successfully merging this pull request may close these issues.

4 participants
0