Probably a design error in FileSystem example...

in Bug Reports
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:
Or I'm missing something and the present code is ok ?
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 ?
0
Comments
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.
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...
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.
is meaningless because it uses same complex locks as implementation in 31.7.1 of ice manual, only code from _facory->remove which is: 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 ?
Cheers,
Michi.