Add support for async db calls

[suirad]

This should defer some of the db load from common player commands to an async task.

[CivcraftBot]

Can one of the admins verify this patch? Type 'ok to test' to test.

[ProgrammerDan]

So beyond the specific comments;

in general I'm afraid this is a good brush, a great brush, applied too broadly. It's generally speaking a bad idea to force every single plugin using Namelayer to be using the "async" versions of the DB calls; you don't know for sure what they are doing, and a much better technique is to leave the API behavior alone for existing methods and add new wrappers that are async, that Namelayer can use internally (and we can test and assert that its safe to do so).

This leads to the final critique: the naming convention is backwards; methods marked "async" are actually the synchronous versions, whereas the version w/o "async" in the name always spawn an asynch thread. Bit confusing.

Otherwise good effort, ping me when you've had a chance to react to / address some of the issues I've raised.

[suirad]

@ProgrammerDan To address all of your mercury notes, I consolidated the check into the message method: https://github.com/Civcraft/NameLayer/pull/208/files#diff-bd66cb6ca613984f83e554cd4a94dfeeR12

The reason i removed most(if not all) of the invalidations, is because the invalidations tells all other servers to recache that group. It seems like most, if not all changes to groups caused a recache message, which would result in a (dbcall * number-of-servers). If we can expect all other servers to reliably receive the recache message, then we can also expect it to reliably receive the messages to just apply whatever changes happened to its own cache. To support this I went around to each part that caused a dbwrite and paired with it a mercury call(obv if available). Example here: https://github.com/Civcraft/NameLayer/pull/208/files#diff-c69a82c6840d6af9abc1feda6fc3086fR161 New mercury message handling here: https://github.com/Civcraft/NameLayer/pull/208/files#diff-1f88aaa5d64cab1c0f1a4bed8eff54d8R47 So with the above changes, when mercury gets a merge message, it will call the same method that would be called locally, the only difference is the 'savetodb' boolean that will disable the db call and mercury message for that call. All those recaches must be choking the servers.

Yea I can see what you mean about the async naming being confusing. For the functionality of it, perhaps making a config option for the async db calls to kick in would be ideal? Really the achievement of the async calls is pushing off db calls that don't return(most writes) anything to the caller to another thread to keep those calls fast and to keep the tick flowing.

[ProgrammerDan]

Ideally you would do as I described; keep the published API behavior unchanged(synchronous) but internally alter all NameLayer owned calls to use the asynchronous method.

[ProgrammerDan]

@suirad Any chance of altering default as described? Again, looks like you did a great job in this pull, but I'm not convinced it's a good idea to force everything that leverages NameLayer to use async DB calls at this point.

Instead, use the async inside namelayer exclusively but leave the previously exposed methods behaving equivalent to prior (sync) to prevent regressions.

I'll correct it in this PR tonight most likely if I haven't heard back from you by then.

[suirad]

That's essentially what was changed. The published api is the same, so I didn't modify any call sites; but internally each write pushes the db call off to an async task.

[ProgrammerDan]

That, specifically, is what I have an issue with. Previously plugins could be sure that the actual DB action was synchronous; at the return of the method call, the DB insert / update was done as well.

Now, that isn't the case.

This is not a good practice.

For external plugins to continue leveraging this synchronous behavior, they now have to call the method name + confusingly "async".

It's just backwards. Easy fix. Code is good, method names are bad. Don't swap out synchronous behavior for asynchronous behavior in publicly accessible methods on longstanding code-bases where many plugins leverage those methods, you will break things.

It's fine to convert all internal uses to async, but the new method name should be the async behavior, not the old method name.

[suirad]

Ah okay that makes sense. I shall adjust it when I get the time. Will probably be tomorrow night.