Remove DB Configuration Locking#791
Remove DB Configuration Locking#791KyleSanderson merged 7 commits intoalliedmodders:masterfrom Headline:hi-dad
Conversation
core/logic/Database.cpp
Outdated
| if (strcmp(current->name.chars(), name) == 0) | ||
| { | ||
| return &conf; | ||
| current->AddRef(); |
There was a problem hiding this comment.
Without this line, we'll get heap corruption for when info in the snippet below goes out of scope. While this does solve this crash, it is fundamentally wrong. I'm not sure why this fixes the problem. Help.
There was a problem hiding this comment.
When you're invoking GetDatabaseConf you're not calling AddRef without this line, but you're calling Release by hand in the caller. Pair this with RAII or match the calls, otherwise you're going to get the assert this is probably generating for you.
core/logic/DatabaseConfBuilder.cpp
Outdated
| #define DBPARSE_LEVEL_DATABASE 2 | ||
|
|
||
| DatabaseConfBuilder::DatabaseConfBuilder() : m_CurInfoList(new ConfDbInfoList()), | ||
| m_InfoList(new ConfDbInfoList()) |
KyleSanderson
left a comment
There was a problem hiding this comment.
oh, this never actually went out last night.
|
@dvander here's your reminder :p |
| ConfDbInfoList *m_ParseList; | ||
| private: | ||
| ke::AString m_Filename; | ||
| ConfDbInfoList *m_InfoList; |
There was a problem hiding this comment.
This (and m_ParseList) should be in a RefPtr.
There was a problem hiding this comment.
note for people driving by: This review comment is also outdated
core/logic/DatabaseConfBuilder.cpp
Outdated
| : m_ParseList(nullptr), | ||
| m_InfoList(new ConfDbInfoList()) | ||
| { | ||
| m_InfoList->AddRef(); |
There was a problem hiding this comment.
Drop this manual AddRef + Release
This is part 1/n in regards to this PR's rework
|
ACK, this should be much safer now w/ the thread lookup removed. |
|
Some simple SQL tests produce expected results. Just another pair of eyes and we should be good to go. I'd request a review, but tagging is the best I can do. @asherkin can you give this a look |
|
Without the refcounting, and with the expected use being something like: I do not see how this could be safe. It would be perfectly legal for the main thread to be reloading the database config, preempt a worker thread between those two lines and trash the returned The cleaner organization and ownership is good, but I think this (in its current iteration) has possibly removed the safety along with the locking, rather than leaving the safety behind. #changemyview EDIT: Threaded access to the config list was removed, this is all wrong. |
| DatabaseInfo info; | ||
| }; | ||
|
|
||
| class ConfDbInfoList : public ke::Vector<ConfDbInfo *> |
There was a problem hiding this comment.
Ok, scratch that last comment - I see that threaded access was removed - but it does look like we leak every ConfDbInfo created?
As @KyleSanderson asked, I attempted to remove configuration locking in favor of a refcounted structure. I'm a multi-threading novice, and this should be heavily reviewed before any approval.
The idea behind this is that different threads can ask for a specific config, and that configuration file it returns will be grabbed from the latest parse. Because we now have two lists in
DatabaseConfBuilder, any search will always happen on the fully populated list. The other one is just used while parsing happens, and it's ptr is copied over to the other member variable once parsing is done. A new list is created in it's place.