Bug 2590 – Deallocator is not called if constructor fails.

Status
RESOLVED
Resolution
WONTFIX
Severity
normal
Priority
P2
Component
dmd
Product
D
Version
D2
Platform
All
OS
All
Creation time
2009-01-17T06:50:24Z
Last change time
2019-09-08T17:47:49Z
Keywords
wrong-code
Assigned to
No Owner
Creator
Max Samukha

Comments

Comment #0 by samukha — 2009-01-17T06:50:24Z
The following test case leaks memory: import std.c.stdlib; class C { new(size_t size) { writefln("In new"); return malloc(size); } this() { writefln("In constructor"); throw new Exception("Exception in ctor"); } ~this() { writefln("in destructor"); } delete(void* p) { writefln("In delete"); free(p); } } void main() { try { auto c = new C; } catch {} } ---- In new In constructor Deallocator should be called to free the memory.
Comment #1 by clugdbug — 2010-04-04T07:43:39Z
I'm pretty sure this bug is invalid. http://herbsutter.com/2008/07/25/constructor-exceptions-in-c-c-and-java/ As Herb says, "destructors only run on successfully constructed objects."
Comment #2 by samukha — 2010-04-04T10:07:51Z
He is talking about constructors/destructors, not allocators/deallocators. I totally agree the destructor must not be called on a partially constructed object. Conversely, the memory that has been successfully allocated for an object needs to be properly deallocated even if the constructor fails. On the other hand, if overloaded new/delete are going to be removed from the language, the problem will pass away.
Comment #3 by Justin.SpahrSummers — 2010-04-04T13:35:10Z
(In reply to comment #2) > He is talking about constructors/destructors, not allocators/deallocators. I > totally agree the destructor must not be called on a partially constructed > object. Conversely, the memory that has been successfully allocated for an > object needs to be properly deallocated even if the constructor fails. > > On the other hand, if overloaded new/delete are going to be removed from the > language, the problem will pass away. When one uses class allocators/deallocators, it's basically like taking memory management into one's own hands. The current behavior seems reasonable, because 'delete' is never invoked on the object (which is necessary for a custom deallocator to be used).
Comment #4 by samukha — 2010-04-04T15:17:17Z
> The current behavior seems reasonable, because > 'delete' is never invoked on the object (which is necessary for a custom > deallocator to be used). I don't think it is reasonable. Why should I bother to call the deallocator manually while the allocator is called automatically? Think about object creation as a transaction consisting of two operations: allocation and construction. If construction fails, allocation should be automatically rolled back.
Comment #5 by Justin.SpahrSummers — 2010-04-04T16:09:04Z
(In reply to comment #4) > > The current behavior seems reasonable, because > > 'delete' is never invoked on the object (which is necessary for a custom > > deallocator to be used). > > I don't think it is reasonable. Why should I bother to call the deallocator > manually while the allocator is called automatically? Think about object > creation as a transaction consisting of two operations: allocation and > construction. If construction fails, allocation should be automatically rolled > back. But the allocator is *not* called automatically, strictly speaking. 'new' is your call to the allocator. Since you use malloc() instead of the garbage collector, 'delete' then becomes necessary. Under normal circumstances, an exception thrown during construction wouldn't leak memory because the garbage collector would eventually collect it; in your code, you took on the task of manually allocating and deallocating memory for objects of class C. It makes sense to me that such custom allocation would entail finer management of exceptional situations.
Comment #6 by schveiguy — 2010-04-05T04:43:18Z
(In reply to comment #5) > > But the allocator is *not* called automatically, strictly speaking. 'new' is > your call to the allocator. Since you use malloc() instead of the garbage > collector, 'delete' then becomes necessary. > > Under normal circumstances, an exception thrown during construction wouldn't > leak memory because the garbage collector would eventually collect it; in your > code, you took on the task of manually allocating and deallocating memory for > objects of class C. It makes sense to me that such custom allocation would > entail finer management of exceptional situations. Not that I disagree this bug is obsolete, but what would you call delete on? With the failed construction, you never got a pointer to the class data. If class allocators were to be saved, I think the correct behavior on a failed constructor should be to call the deallocator.
Comment #7 by Justin.SpahrSummers — 2010-04-05T19:14:23Z
(In reply to comment #6) > > Not that I disagree this bug is obsolete, but what would you call delete on? > With the failed construction, you never got a pointer to the class data. > > If class allocators were to be saved, I think the correct behavior on a failed > constructor should be to call the deallocator. Fair point. I didn't realize that it's impossible to hold onto the pointer.
Comment #8 by samukha — 2010-11-27T12:03:42Z
Though D is going to deprecate new/delete operators, it is worth noting that the rule under discussion was adopted by C++ 15 years ago. From "Counting Objects in C++" article by Scott Meyers (http://blog.csdn.net/LYH_Studio/archive/2006/08/11/1051927.aspx): "For many years this was a hole in the draft C++ language specification, but in March 1995 the C++ Standards committee adopted the rule that if, during a new expression, the invocation of operator new succeeds and the subsequent constructor call throws an exception, the runtime system must automatically deallocate the memory that operator new allocated. This deallocation is performed by operator delete, the deallocation analogue of operator new."
Comment #9 by samukha — 2010-11-27T12:06:43Z
A better formatted text: http://www.drdobbs.com/cpp/184403484
Comment #10 by pro.mathias.lang — 2019-09-08T17:47:49Z
While the bug report seems valid, allocator / deallocators are deprecated, hence going to close this.