On Tue, Jun 22, 2010 at 09:48:29AM -0400, Sam Carleton scratched on the wall: > On Tue, Jun 22, 2010 at 9:15 AM, Pavel Ivanov <paiva...@gmail.com> wrote: > > > > The idea is that the copy and nulling happens very quickly where the > > > sqlite3_close is more expensive, do the copy/null very quickly so that if > > > another thread calls Close during the first threads execution of > > > sqlite3_close, the second thread finds m_db null and will not call > > > sqlite3_close. > > > > Wow, if you really relied on that then you did too few of > > mutli-threaded programming.
> Yea, I know it isn't as safe as it should be, but in this case it is one > class per thread. Why I am doing it, I don't know, old habit that I just > cannot seem to break. In light of your next statement, I think I will > change it;) The practice is justified, as "get it done quickly" has nothing to do with threads. It is a defensive programming style that is based off invariants: <http://en.wikipedia.org/wiki/Invariant_%28computer_science%29>. In this case, the one absolute thing that must happen in ::Close() is that m_db needs to get set to NULL. It would be a good idea to also close and properly destroy the database connection, but that isn't as critical. If the database connection isn't actually closed, the application will leak memory and resources. That's bad, but there is a good chance the application will continue to run-- likely until the natural termination of the process, which will clean everything up anyways. However, if m_db is not set to NULL there is, as we saw, a good chance the application will just crash outright. Most people consider this worse, especially if it happens on a customer's machine (the common case for most code execution). So the very first thing the function does is set m_db to NULL. It then uses a locally scoped copy of the database handle to actually try to get some work done (like call sqlite3_close()). But since this variable is locally scoped, we know it will go out of scope at the end of this function. This means that, one way or another, no matter how the function exits, neither the class nor any other variable in the application will have a stale reference to the database handle. This greatly reduces the possibility of a crash. In fact, the only stale pointer that ever exists is the locally scoped copy, which has a very short and confined lifetime. If the function does something wrong, the database connection might not actually be closed, but that's true regardless of how you structure the function. In a function this small it might seem a bit silly, but consider a 100 line function that has the "m_db = NULL;" statement at the bottom. All it takes is someone to come back to code three months later, do a quick scan, and add a "return;" half way through the function to deal with a hopeless error condition (this being called from a distructor makes any serious action difficult). The same general ideas apply to called client functions (e.g. stuff like sqlite3_close()) that might throw an exception, kill the thread, or otherwise disrupt the normal code flow. To defend against this, we do the important stuff "quickly," before any other code (and, in specific, external functions) has a chance to screw something up-- not because of concurrency, but because we don't trust the code (and the functions it calls) that may run next. You can say that this just points to bigger structure and error handling issues, and you might be right. This is just a safety net, not an end-all solution. But this is still a really good habit, especially for situations similar to COM code that is likely wrappering other code or libraries not under the control of the developer writing this code. This style provides a line of defense against poorly written libraries, as well as future-proofing against libraries that may be changed or updated independently of the wrapper code. There are those that might say crashing is better than silently failing, as it is more "in your face" and demands attention (and a fix). That's only true while in development, however, where there is a chance to notice it, analyze it, and fix it. A customer crash is almost always a worst-case situation. Even in-development, if you're using tools like Valgrind, the any issue with program logic or flow control is likely to show up. A Valgrind report should put up just as big a red flag as a crash-- and is likely to be easier to debug. The biggest problem with invariant style programming is that, quite frankly, it is very hard. You also need very strict and specific requirements for each function. Unless you've been doing it for some time, it might seem odd that setting the pointer to NULL is an absolute requirement of the ::Close() method, while actually closing the database connection is not. You can learn patterns, but unless you understand the why and how of the situation, it is of limited value and just looks funny. In short, it is a situation of "smart programming as a defense against poor programming (and poor environments)." Unfortunately, if there is a need to defend against poor programming, that's just as likely to be true here as it is there, meaning that the invariant style scales poorly across a larger development environment (read: one that inevitably has below-average programmers). It also adds enough complexity that a net positive gain in stability and reliability is somewhat questionable. Invariants are a powerful tool, and they have a strong place in computer science to write verifiable algorithm proofs. But like any powerful tool, they're easy to misuse. Their day-to-day practical value on a large scale programming project is still, IMHO, an unanswered question. -j -- Jay A. Kreibich < J A Y @ K R E I B I.C H > "Intelligence is like underwear: it is important that you have it, but showing it to the wrong people has the tendency to make them feel uncomfortable." -- Angela Johnson _______________________________________________ sqlite-users mailing list sqlite-users@sqlite.org http://sqlite.org:8080/cgi-bin/mailman/listinfo/sqlite-users
