www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - Synchronized object must not be deleted. causes useless error message

reply BCS <BCS pathilink.com> writes:
this

auto o = new Object;
synchronized(o)
	delete o;

causes a runtime error:

Assertion failure: 'h->monitor" on line 99 in file 'internal\monitor.c'
Nov 21 2006
next sibling parent Sean Kelly <sean f4.ca> writes:
BCS wrote:
 this
 
 auto o = new Object;
 synchronized(o)
     delete o;
 
 causes a runtime error:
 
 Assertion failure: 'h->monitor" on line 99 in file 'internal\monitor.c'

This is to be expected, as 'delete' destroys the object monitor so it no longer exists when the synchronized block completes. I would consider the above to be programmer error, and a runtime message is the most appropriate response for this. Sean
Nov 21 2006
prev sibling parent reply Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
BCS wrote:
 this
 
 auto o = new Object;
 synchronized(o)
     delete o;
 
 causes a runtime error:
 
 Assertion failure: 'h->monitor" on line 99 in file 'internal\monitor.c'

So what would you prefer to happen? Short version of the problem: because of the synchronized block you're accessing a dangling pointer to o. Long version: When you exit a synchronized block, it needs to unlock the monitor associated with the object. I guess before the object is deleted the monitor pointer is set to null (like the vtable pointer), which is a good thing because otherwise you'd be accessing invalid memory without knowing it. You're still accessing invalid memory of course, since the monitor pointer is now itself in invalid memory, but at least it's been detected and you're told about it.
Nov 21 2006
parent reply BCS <BCS pathilink.com> writes:
Frits van Bommel wrote:
 BCS wrote:
 this

 auto o = new Object;
 synchronized(o)
     delete o;

 causes a runtime error:

 Assertion failure: 'h->monitor" on line 99 in file 'internal\monitor.c'

So what would you prefer to happen?

Except that I was testing to see what would happen in that case, I'm not sure I would have any clue what was wrong. options (in no special order): 1> objects that are "locked" can't be deleted. 2> deleting a locked object, unlocks it. :b 3> some sane error message at either the delete or unlock Error: can't delete locked object. or Error: can't unlock deleted object. problems: #1 thread 1: synchronized(o) while(true){go = true;} thread 2: go = false; while(!go){}; delete o; // fail #2 threads 1-n competing on this: synchronized(o) { auto a = o.getChild(); wait(); a.foo; // more than one thread could get this } thread 0: delete o; #3 what should it say? Yes, it is a bug, but the error response is not good.
Nov 21 2006
parent Frits van Bommel <fvbommel REMwOVExCAPSs.nl> writes:
BCS wrote:
 Frits van Bommel wrote:
 BCS wrote:
 this

 auto o = new Object;
 synchronized(o)
     delete o;

 causes a runtime error:

 Assertion failure: 'h->monitor" on line 99 in file 'internal\monitor.c'

So what would you prefer to happen?

Except that I was testing to see what would happen in that case, I'm not sure I would have any clue what was wrong. options (in no special order): 1> objects that are "locked" can't be deleted.

That may be pretty easy to implement, actually. You'd just need to add an extra function to check if the monitor is locked to internal/monitor.c, then call that from _d_delclass in internal/gc/gc.d.
 2> deleting a locked object, unlocks it. :b

Since it's always a bug to delete a locked object (i.e. one that's currently in use) it's better to just produce an error message.
 3> some sane error message at either the delete or unlock
     Error: can't delete locked object.
     or
     Error: can't unlock deleted object.

Yeah, the error message could have been better. I guess Walter didn't take into account the fact that someone might be stupid enough to delete a locked object when he wrote that code :P.
 problems:
 
 #1
 
 thread 1:
     synchronized(o)
         while(true){go = true;}
 
 thread 2:
     go = false;
     while(!go){};
     delete o;    // fail
 
 #2
 
 threads 1-n competing on this:
     synchronized(o)
     {
         auto  a = o.getChild();
         wait();
         a.foo;    // more than one thread could get this
     }
 
 thread 0:
     delete o;
 
 #3
     what should it say?
 
 
 Yes, it is a bug, but the error response is not good.

Like I said, the message could be better. Now that I think about it, maybe it should also be delivered by throwing an exception instead of just asserting... Or maybe not, since that would make it too easy to suppress accidentally (there are probably still some people who use catch-all exception handlers out there)
Nov 21 2006