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:



thread 1:
	synchronized(o)
		while(true){go = true;}

thread 2:
	go = false;
	while(!go){};
	delete o;	// fail



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;


	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:
 

 
 thread 1:
     synchronized(o)
         while(true){go = true;}
 
 thread 2:
     go = false;
     while(!go){};
     delete o;    // fail
 

 
 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;
 

     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