py: Clear finalizer flag when calling gc_free. #1595
Conversation
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.
|
Very nice work! But shouldn't in this case the finaliser be called? (ie when finalisable memory is explicitly free'd) |
|
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 The only place that I could find that calls m_new_obj_with_finaliser and explicitly frees it is in stmhal/file.c: 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). |
|
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). |
|
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 :) |
|
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". |
I think this makes sense. |
+1 So, I'll take @dhylands' fix and add that comment. |
|
Merged in 7f3c0d1. |
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.