 |
Move to 2.11 ? |
 |
Looking at the description, the race condition happened while registering cvar. But in reality all cvars are global variables, and they are registered during global CRT initialization in single thread: no race condition is possible in this case! And after CRT initialization, the set of existing cvars stays unchanged, so there is no problem searching for them without lock. Perhaps this crash happened because sometimes we used nasty approach of making a cvar static variable inside a function. Such a cvar is initialized only when the function is first executed, thus: 1) User cannot see/find it when game starts: it does not exist yet. 2) If function is called during gameplay in parallel section, then race condition happens. I stopped doing this because of p.1, but I guess now we have a more important reason to avoid such practice. I only found one such cvar right now, so I fixed it: r10707 Cvar r_maxTri renamed to r_maxTrisPerDrawCall and made global variable. I guess it also explains why I have not seen any similar crashes in a while. |
|
|
So let's repeat: 1) It is ok to define idCVar as global variable in cpp file. It does not matter if it has static keyword or not. 2) It is ok to define idCVar as static class member. 3) It is NOT ok to define idCVar as non-static class member (it would be initialized only when object is created). 4) It is NOT ok to define idCVar as static variable inside function (it would be initialized only when function is called). |
|
|
I vaguely recall that this issue was at one point considered to be the culprit behind some arrow crashes. The only one I could reliably repeat was in "The Transaction" Since this change, I have yet to repeat the crash again. I hope this is the end of that saga. |
|
|
I heavily doubt that the arrow crash is caused by threadsafety of cvars =) |
|
|
I'm not gonna argue on that since my whole statement was a vague recollection. I think the premise may have revolved around parsing tdm_weapon_next_on_empty during the bow loop but I'll table anything more on this until I find the old convo. |
|
|
I cleaned cvars system before trying to implement cvar mission overrides: https://forums.thedarkmod.com/index.php?/topic/22428-internal-cvar/ These commits are for merging, i.e. remove internal cvar and cvars sharing: r10713 Merged idInternalCVar into idCVar. r10714 Cleanup after recent removal of idInternalCVar. And these later cvars are about threadsafety: r10715 Removed "modifiedFlags" state from idCVarSystem. r10716 Disabled dynamic creation of unknown/unregistered cvars. r10717 Described thread-safety conditions in description header. I think I have reviewed this code rather thoroughly now and am quite confident in what is OK and what is not. Here is the citation from CVarSystem.h: stgatilov 0005600: CVars must be declared as global variable or as static class member. Unlike original Doom 3, a cvar can only have one C++ variable connected to it. stgatilov 0005600: Standard thread-safety principles apply: * It is OK to read/write different cvars in parallel. * It is OK to read the same cvar in parallel. * It is WRONG to write cvar in parallel with reading/writing the same cvar. * Some methods are only called during initialization, they are never in parallel to anything. |
- Anonymous
