Add is_closed() to Database class

[Joost Cassee]

It would be nice to be able to check whether the database has been closed, especially in the light of the dire words in the close() method documentation.

Please add an is_closed() method to the Database class.

[Olly Betts]

Um, "dire words"?

The issue here is that we want to allow API methods to efficiently cache data. So after you close a database, either an operation fails with an exception (which you can catch and deal with), or it returns the correct result because it is using data fetched from the database before it was closed.

The alternative is to gate every single API method which involves use of a Database in any way (which is most of them) with a check if the Database has been closed, and if it has then force it to fail. That's a lot of extra work for no clear benefit.

Can you explain how you read the documentation here? It makes sense to me, but we ought to fix it if it can mislead.

Since there are no dire consequences, I'm undecided if this is a useful method to add. It's not too hard to do, but there are various costs to adding more methods.

[Richard Boulton]

In all the usage patterns I've used to access Xapian databases, keeping track of "is_closed()" for a database is easy to do outside Xapian (or is irrelevant, because the reference to the database object is dropped immediately after calling close()), so I'm also unsure that there is much justification for adding an is_closed() method.

However, one situation where it could be useful is for checking if an exception has caused the database to become closed. IIRC, Some exceptions (eg, ones which are reporting database corruption) will close the database, but others (eg, "document not found" exceptions) will not. Users might reasonably want to implement a generic error handler which checks if the database is now closed, and takes some appropriate action if so.

[Joost Cassee]

I meant 'dire' in a tongue-in-cheek way. I was reacting to the "this may happen or that may happen, and you will not know which one" feel of the text. I agree with Olly that the behaviour is consistent and necessary for performance reasons. It is something I had to think about, though, and I thought "let's just not use the database when it is closed".

A search function that uses Xapian will be a relatively expensive action, and in that case a check whether the database is still open is relatively cheap. I was thinking that such function would have to keep a 'database is closed' flag if it were interested in that fact. And then I thought, what code knows better whether the database is closed than the database itself? Why would I not be able to ask the database whether it is closed?

I may have come on a bit strong in my ticket description, sorry. The lack of an is_closed() method seemed an oversight, but I had not considered that even a small change means an investment of your time and must add real value.

A suggestion to improve the documentation of the close() method would be to reduce the 'heavyness' of the second half:

"After this call, calls made to methods of the database will either behave exactly as they would have done if the database had not been closed (if the method uses cached data) or raise a Xapian::DatabaseError indicating that the database is closed."

Too much information here indicates that the caller must be very cautious, which is not the case.

[Dan]

This might have some merit in bindings or client libraries, but I'm not convinced its useful in the main library. Mostly, I'd like to point out again that if the database is in fact closed, the reference to that database is dropped. I'm going to move this out of the library API and into bindings for further consideration.

[Olly Betts]

I adjusted the documentation comment for Database::close() in r15893 to tone down the warning a bit, and also added a rationale for why we have this behaviour.

jcassee: The time required to implement isn't a big issue (though it is a consideration). My main concern is that the more methods a class has, the harder it is for users to get to grips with using it - the cost of a new method is that it makes the class a little harder to comprehend. As Richard says, my experience is that it's easy to track this outside Xapian (generally you actually have a single handle on the Database and can just set it to NULL/undef/nil/etc after you close it).

dcolish: How would you implement an is_closed() type method in the bindings or a client library without support in the C++ API?

[Olly Betts]

Backported the doc comment improvements for 1.2.7 in r15966.

[Olly Betts]

I think what Richard said in comment:2 is also true for code written using the bindings. I also asked dcolish on IRC and he's happy to close this.