A few sourcecode questions based on my previous work

grembo

Hi,

I've been comparing my patches to Ice-3.4.2 for C++11 and libc++ with Ice-3.5.0 and have a few questions/remarks. Please correct me if I'm wrong.

Freeze/MapDb destructor throws
The destructor of this class throws an exception, which - when compiled with C++11 - will abort execution immediately, since it cannot leave the destructor. The destructor cannot be marked "noexcept(false)" easily, since it inherits from class Db of Berkley Db, which uses the strict default exception specification for destructors. In my patch to Ice-3.4.2 I replaced the throw with an error log:

--- cpp.orig/src/Freeze/MapDb.cpp       2011-06-15 21:43:58.000000000 +0200
+++ cpp/src/Freeze/MapDb.cpp    2012-09-10 11:43:58.000000000 +0200
@@ -72,7 +72,13 @@ Freeze::MapDb::~MapDb()
         }
         catch(const ::DbException& dx)
         {
+#if defined(ICE_DESTRUCTORS_DONT_THROW_BY_DEFAULT)
+            Error out(_communicator->getLogger());
+            out << "DbException while closing database " << _dbName << ": "
+                << dx.what();
+#else
             throw DatabaseException(__FILE__, __LINE__, dx.what());
+#endif
         }
     }
 }

I'm not 100% sure who's supposed to catch the exception and if replacing it with a warning is sufficient. If it isn't I'd suggest to terminate the program with a more helpful error message instead. The current state of affairs might make it really hard to figure out the reason of a crash.

Potential destructor problem in Freeze/ObjectStore
Similar to the issue above, the destructor of ObjectStoreBase contains the following code:

Freeze::ObjectStoreBase::~ObjectStoreBase()
{
    try
    {
        _db->close(0);

        for(size_t i = 0; i < _indices.size(); ++i)
        {
            _indices[i]->_impl->close();
        }
        _indices.clear();
    }
    catch(const DbException& dx)
    {
        Ice::Error error(_communicator->getLogger());
        error << "Freeze: closing ObjectStore " << _dbName << " raised DbException: " << dx.what();
    }
}

Which looks good at first glance, bit checking IndexI::close I see

void
Freeze::IndexI::close()
{
    if(_db.get() != 0)
    {
        try
        {
            _db->close(0);
        }
        catch(const DbException& dx)
        {
            throw DatabaseException(__FILE__, __LINE__, dx.what());
        }
        _db.reset(0);
    }
}

So in case calls to IndexI::close fail (e.g. DbDeadLockException), the DbException is rethrown as a Freeze::DatabaseException which then will leak Freeze::ObjectStoreBase::~ObjectStoreBase which in turn will cause the application to terminate.

I'd suggest multiple possible solutions here:

1. If the design permits, don't rethrow in IndexI::close, so DbException will get logged properly.
2. Also catch for DatabaseException in ObjectStoreBase::~ObjectStoreBase and log it like it's done for DbException.
3. Mark IceUtil::Cache::~Cache as well as ObjectStoreBase::~ObjectStoreBase as noexcept(false). This only makes sense if the design requires the exception to leave the destructor.

IceGrid/FileCache::read might report wrong newOffset
The following code fragment might be problematic, since newOffset is not increased by line.size(), when there was a partial successful read. This might be corner case, but it's easy to fix.

Original code:

        if(is.eof())
        {
            newOffset += line.size();
        }
        else if(!is.fail())
        {
            newOffset = is.tellg();
        }

I think this should be changed to:

        if(is.eof() || is.fail())
        {
            newOffset += line.size();
        }
        else
        {
            newOffset = is.tellg();
        }

Cheers,
Michael

bernard

Hi Michael,

We added a few ICE_NOEXCEPT_FALSE to Ice 3.5.0, for C++11 support. I don't think any additional ICE_NOEXCEPT_FALSE is needed, in particular the Freeze destructors you mentioned should never throw, because the DB close() calls never fail.

I actually checked with Oracle support, and I don't believe that with Freeze there is any scenario where commit() or close() could raise a deadlock exception, which is the only exception that could be thrown.

Nevertheless, we plan to review this code for 3.5.1, at least for readability; in practice it's not a concern - no exception is ever thrown here.

We will also review the FileCache::read code you mentioned.

Thanks,
Bernard