Probably a design error in FileSystem example...

Sheff

Hello, I've looked through the lifecycle FileSystem example and I think there's
a design issue that allows executing operation on an already destroyed ice object.
Consider this scenario:
We have an empty directory and 2 threads:
1.Thread A calls DirectoryI::createDirectory locks mutex m_, asserts that object isn't destroyed and unlocks m_, at this point scheduler starts thread B.
2.Thread B calls DirectoryI::destroy, since there're no files in directory it is removed from object adapter, added to reap map and marked as 'destroyed'.
3.Thread A continues execution, locks lcMutex_, creates and activates new directory with an already destroyed parent.

as a solution, i think we should write:

if(_destroyed)
{
            throw ObjectNotExistException(__FILE__, __LINE__, c.id, c.facet, c.operation);
}

right after

IceUtil::StaticMutex::Lock lock(_lcMutex);

in DirectoryI::createDirectory and all other similar functions

Or I'm missing something and the present code is ok ?

michi

Hi Stanislav,

thanks for pointing this out. It looks like you are right. It's indeed possible for the new directory to be created with a parent that was just destroyed. Re-testing the _destroyed flag as you suggest would fix this.

Thanks for the bug report, I'll update the demos and the doc.

Cheers,

Michi.

Sheff

Thanks.
By the way, I really don't see why use reaping in this example, consider
replacing "_parent->addReapEntry(_name)" with "_parent->_contents.erase(_name)"
and remove all "reap()" and you'll get in fact the same code plus no reaping, i.e no garbage in server...

michi

Yes, it could be implemented that way. However, in general, for applications with complex inter-dependencies between objects, ensuring that immediate destruction of object is deadlock free can be quite difficult.

We chose to illustrate the reaping technique for that reason: it makes it easier to write code that is provably free of deadlocks and is one of the important design patterns you should have in your toolbox when writing Ice applications.

Cheers,

Michi.

Sheff

Yes, I understand, but what I meant to say was that in this example reaping
is meaningless because it uses same complex locks as implementation in 31.7.1 of ice manual, only code from _facory->remove which is:

Mutex::Lock lock(_lcMutex);
try
{
    a->remove(a->getCommunicator()->stringToIdentity(name));
} catch(const NotRegisteredException&)
{
    throw ObjectNotExistException(__FILE__, __LINE__);
}
_names.erase(name);

has been moved to "destroy()" methods of FileI and DirectoryI and instead of
calling _names.erase() it calls addReapEntry();
My point is that reaping in filesystem example uses the same set of locks and in the same order as "complex implementation without reaping" so there's no point in such reaping.
Simple reaping as described in 31.7.2 is "real reaping" because PhoneEntryI::destroy() doesn't acuire lcMutex and it doesn't depend on PhoneEntryFactroy. In fileystem example "FileI::destroy()" and "DirectoryI::destroy()" both acquire lcMutex and thus depend on their "factory",
so, as you can see, interobject dependency didn't go away as it supposed to, it's still there, so I guess we can't call that "real reaping"... or I'm missing something ?

michi

The point here is that the implementation avoids the upcall from the child to the parent to unhook the child from the parent when the child is destroyed. That kind of circular dependency is very deadlock-prone. I agree that it is possible to implement the file system demo without reaping and still have it deadlock free. But, the more complex the inter-relationships between objects, the harder it gets to find such a solution and reaping is generally simpler.

Cheers,

Michi.