Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove DB Configuration Locking #791

Merged
merged 7 commits into from Jun 19, 2018

Conversation

@Headline
Copy link
Member

Headline commented Mar 28, 2018

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.

@Headline Headline force-pushed the Headline:hi-dad branch from c27af00 to 53dde59 Mar 28, 2018
{
return &conf;
current->AddRef();

This comment has been minimized.

Copy link
@Headline

Headline Mar 28, 2018

Author Member

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.

https://github.com/Headline/sourcemod/blob/53dde59defa8b6029bc2f716952ee1a43841a2ef/core/logic/Database.cpp#L313-L322

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

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.

#define DBPARSE_LEVEL_DATABASE 2

DatabaseConfBuilder::DatabaseConfBuilder() : m_CurInfoList(new ConfDbInfoList()),
m_InfoList(new ConfDbInfoList())

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

what happened here?

SMCStates states = {0, 0};
if ((err = textparsers->ParseFile_SMC(m_Filename.chars(), this, &states)) != SMCError_Okay)
{
logger->LogError("[SM] Detected parse error(s) in file \"%s\"", m_Filename);

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

.chars() since you converted to ke::AString

@Headline Headline force-pushed the Headline:hi-dad branch from a151b0f to 9d01dc6 Mar 28, 2018
Copy link
Member

KyleSanderson left a comment

oh, this never actually went out last night.


void DatabaseConfBuilder::ReadSMC_ParseEnd(bool halted, bool failed)
{
m_InfoList->Release();

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

This isn't thread-safe. You need to release after you replace the ptr.

This comment has been minimized.

Copy link
@Headline

Headline Mar 28, 2018

Author Member

How is this not thread-safe, exactly? We're ++refcount on it's creation and when we replace the reference a line down we'll no longer be able to --refcount, resulting in a never-destroyed obj

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

There needs to be a guarantee that m_InfoList is never invalid, once you release the final reference (this) obviously becomes invalid memory. You need to store infoList in the function, replace the ptr, and then perform the release. Alternatively, see if you can change this type to be a refptr.

{
m_InfoList->Release();
m_InfoList = m_CurInfoList;
m_CurInfoList = new ConfDbInfoList();

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

When does this cleanup? After the parser finishes this should be nullptr?

m_CurInfoList->AddRef();
}


This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

Corrupt EoF.

const char *DBManager::GetDefaultDriverName()
{
return m_DefDriver.c_str();
ke::AString defaultDriver(builder.GetDefaultDriver());
return defaultDriver.chars();

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

Use after free.

{
ConfDbInfo &db = *(*iter);
if (db.realDriver == pDriver)
ConfDbInfo *current = list->at(i);

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

You want a ke::RefPtr here.

This comment has been minimized.

Copy link
@Headline

Headline May 8, 2018

Author Member

note for people driving by: This review comment is outdated since we threw out the refptr stuff


if (m_ParseState == DBPARSE_LEVEL_DATABASE)
{
ConfDbInfo *cdb = new ConfDbInfo();

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

Do we need to go through this entire process? Can't we just take the existing ptr and toss it in?

return current;
}
}
return NULL;

This comment has been minimized.

Copy link
@KyleSanderson
cdb->info.host = cdb->host.chars();
cdb->info.user = cdb->user.chars();
cdb->info.pass = cdb->pass.chars();
{

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

whitespace.

@Headline Headline force-pushed the Headline:hi-dad branch from 447a96c to 2cb1481 Mar 28, 2018
temp->Release();

m_CurInfoList = nullptr;
}

This comment has been minimized.

Copy link
@KyleSanderson
m_InfoList = m_CurInfoList;
temp->Release();

m_CurInfoList = nullptr;

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

I'd maybe rename these to m_parseInfoList or something and store them differently (in another private:), the cur stuff while I'm used to it now is really unclear to anyone doing a driveby.


/* Save it.. */
m_CurInfoList->append(m_CurInfo);

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

reset or free m_CurInfo here.

m_InfoList->Release();
}

ConfDbInfoList *DatabaseConfBuilder::GetConfigList()

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson Mar 28, 2018

Member

This needs to be refptr'd in the InfoList, bringing it in from the builder still violates the refptr's guarantee. One authoritative data source.

This comment has been minimized.

Copy link
@Headline

Headline Mar 29, 2018

Author Member

I'm not entirely sure I understand, are you saying we should be doing something along the lines of

RefPtr<ConfDbInfoList> list(m_InfoList);
return list;

?

This comment has been minimized.

Copy link
@Headline

Headline Mar 29, 2018

Author Member

Github shows that this review comment is outdated, but no changes involving this have actually been made. Just a heads up

@Headline Headline force-pushed the Headline:hi-dad branch from 2d483b3 to 31e1f44 Mar 30, 2018
@Headline

This comment has been minimized.

Copy link
Member Author

Headline commented Mar 31, 2018

@dvander here's your reminder :p

ConfDbInfoList *m_ParseList;
private:
ke::AString m_Filename;
ConfDbInfoList *m_InfoList;

This comment has been minimized.

Copy link
@dvander

dvander Mar 31, 2018

Member

This (and m_ParseList) should be in a RefPtr.

This comment has been minimized.

Copy link
@Headline

Headline May 8, 2018

Author Member

note for people driving by: This review comment is also outdated

: m_ParseList(nullptr),
m_InfoList(new ConfDbInfoList())
{
m_InfoList->AddRef();

This comment has been minimized.

Copy link
@dvander

dvander Mar 31, 2018

Member

Drop this manual AddRef + Release

@Headline Headline force-pushed the Headline:hi-dad branch from 1b3cb14 to ff60c74 May 1, 2018
Headline added 2 commits May 1, 2018
This is part 1/n in regards to this PR's rework
@dvander

This comment has been minimized.

Copy link
Member

dvander commented May 2, 2018

ACK, this should be much safer now w/ the thread lookup removed.

{
friend class DatabaseConfBuilder; // lets DatabaseConfBuilder use SetDefaultDriver
public:
ke::AString& GetDefaultDriver() {

This comment has been minimized.

Copy link
@KyleSanderson

KyleSanderson May 8, 2018

Member

I almost wonder if we even need this exposed publicly still. Code that doesn't handle this will fallback to the default driver anyways; this seems like a well intentioned piece that we just never really used. Errors if a specific databases entry is outstanding just don't really make sense to me.

This comment has been minimized.

Copy link
@Headline

Headline May 8, 2018

Author Member

Are you suggesting to keep the default driver in DBManager where it was before? I like it's existence here that way a ConfDbInfoList contains every piece of information acquired from a single parse

@Headline Headline force-pushed the Headline:hi-dad branch from 6a137b9 to 5a2701b May 8, 2018
@Headline

This comment has been minimized.

Copy link
Member Author

Headline commented May 8, 2018

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

@asherkin asherkin self-requested a review May 8, 2018
@asherkin

This comment has been minimized.

Copy link
Member

asherkin commented May 11, 2018

Without the refcounting, and with the expected use being something like:

ConfDbInfoList *list = m_Builder.GetConfigList();
ConfDbInfo *pInfo = list->GetDatabaseConf(name);

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 ConfDbInfoList before the call to GetDatabaseConf.

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 *>

This comment has been minimized.

Copy link
@asherkin

asherkin May 11, 2018

Member

Ok, scratch that last comment - I see that threaded access was removed - but it does look like we leak every ConfDbInfo created?

@Headline Headline force-pushed the Headline:hi-dad branch from 68bff4f to f3b9587 May 12, 2018
@KyleSanderson KyleSanderson merged commit b9b6832 into alliedmodders:master Jun 19, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Headline Headline deleted the Headline:hi-dad branch Jun 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.