A few comments (but nowhere near a design suggestion)... On Tue, 2009-09-29 at 09:57 -0400, Carolyn Beeton wrote: > An earlier discussion starting at > http://list.sipfoundry.org/archive/sipx-dev/msg17145.html is > interesting, but I don't think we got to the root of the problem in > the commit done in 15104 to fix XECS-2463 > (http://track.sipfoundry.org/browse/XX-4765). The root of the problem > seems to be that the various DB classes are not true singletons, at > least not across processes. We end up with a separate instance (and > lock) in each process, so setting mTableLoaded flags (for example) is > useless to ensure that the xml file is only loaded once. The only > thing which makes the xml file be loaded once (most of the time) is > the check on the number of users of the database > (pSIPDBManager->getNumDatabaseProcesses(name)); but the constructor > fails to load at all if the number of users is not one, and I think > that must be happening sometimes on startup (i.e. two processes are > starting up at the same time, and both constructors see two users, so > don't load).
I think that in principle we could arrange for getNumDatabaseProcesses to return the correct number, if we replaced it with a method to "(1) add this process to the "process table" of processes using this table, and (2) return the number of entries in the process table". The current code does both of these things, but in different methods, each of which is a different DB transaction, so of course there is a race condition. But IMDB does have global transactions, so a single DB transaction should be able to do both the add and the count in a fully interlocked way. > There could also be a problem in writing the file, since the separate > lock in each process does not guarantee that one process will not > begin to write the file while another is also in the process of > writing. That is true, but the actual replacement of the file contents with the new file contents is done by a rename of the temporary output file to the name of the real file, and rename is implemented atomically by the kernel. > - SubscriptionDB::load() will return false if the file does not exist > (this should return success). QUESTION: should this be a special case > for subscription.xml, or does it apply to some of the other files as > well? (specifically registration.xml) That's a fascinating question. What is the defined meaning of the return value of ::load()? If it means "Has the table been successfully initialized?", then the answer should probably be TRUE. OTOH, I think that these ::load() methods are only used by their corresponding constructors, so we can define their semantics to be what is convenient for the implementation. > - I can't reproduce it, but I think the "not loaded" case occurs when > getNumDatabaseProcesses returns more than one user (and since we > initialized mTableLoaded to true, we don't load it unless users==1) > > It seems to me that we need the SubscriptonDB class and probably all > the DB classes to be true cross-process singletons so that there is a > single lock guarding the file and we can guarantee that only one > process will load the file into the IMDB. I think that a method that does the "add and query" as one DB transaction would exploit the IMDB's global transaction lock as the lock you want. Dale _______________________________________________ sipx-dev mailing list [email protected] List Archive: http://list.sipfoundry.org/archive/sipx-dev Unsubscribe: http://list.sipfoundry.org/mailman/listinfo/sipx-dev sipXecs IP PBX -- http://www.sipfoundry.org/
