> I found where we tickle the race condition in db_create_record().
>
> 1) in mhttpd.c, every time we show the status page, we call
> db_create_record(hDB, 0, "/Runinfo", strcomb(runinfo_str));
> 2) internally db_create_record() deletes /RunInfo
> 3) other programs read "/runinfo/run number" while it is deleted do not
> check for the db_get_value() error code and happily get a zero run number.
>
> Stephan fixed the race condition, and now I commited an mhttpd.c change that
> only calls db_create_record(hDB, 0, "/Runinfo", strcomb(runinfo_str)); if
> /runinfo does not exist. This seems to be redundant with a similar call in
> cm_connect_experiment1(), called each time a new client starts up.
The reason for the db_create_record() is the following: Assume that we change
the /runinfo structure, by adding an additional variable in the future. If we
run a "new" mhttpd on an "old" experiment, the "runinfo" C structure does not
match the ODB contents. The db_create_record() ensures that the ODB structure
exactly matches the C structure. I agree with you that this can cause
potential problems. But most of them should be fixed by the additional lock()
I added recently. So other programs cannot read the run number while it is
deleted.
One could think of checking the record size, and re-creating the runinfo if
the ODB record size does not match the C record size. But this does not
prevent the potential error that some variable are reversed in order. They
are then mapped wrongly to the C runinfo structure.
I see that you work very hard now on all possible checks for the run number.
But I would not commit that and make it part of the distribution, since all
experiments at PSI for example do not have this run number problem. Run it
locally, determine the cause of your problem (the discovery of the race
condition was already very good, I'm glad that your found it, should make the
system much more stable), and we'll fix it. Puttin ASSERT's is a good idea, I
should have done it from the very beginning. But if you start now, please put
it in all other 100000 places (;-)
I would not add a db_get_value_cannot_possibly_fail() into the standard
distribution, because it probably cannot correct the initial problem and then
just will go into an infinite loop. We should tackle problems always at their
source.
If you cannot resolve your zero run number problem, do the following: There
is a cm_msg(MDEBUG, ...) which only puts a message into the shared memory,
but not in midas.log. This can be used for real time debugging. Add those
message temporarily in db_get_value() etc. to see what is going on. As soon
as the run number goes to zero, stop all processes immediately (for example
by locking the database with db_lock_database), and the look backwards in the
sysmsg buffer to see what happened *before* the run number went to zero.
- Stefan |