Multistate thread unsafety

[Shauren]

In multistate mode and worldserver map update threads > 1, all exposed functions mutating group and guild are missing synchronization

I'm talking about actions whose regular ingame packet handlers are marked as THREADUNSAFE, forced to only ever happen in main thread such as:

• adding/removing group members
• changing leader
• converting to lfg/raid
• adding/removing guild members
• disbanding
• setting ranks
• changing bank text

Solution could be one of

• add synchronization (this is just bad)
• queue the code for later execution on world thread (queue itself must be threadsafe)
• just remove from registered functions on map bound lua state (maybe not very user friendly)

[Foereaper]

We added support for marking methods as either world only, map only or both. I went through and changed a good amount of these to map/world only, but I probably missed some. I think this would be the best way to handle it, allowing the "global" state to deal with guild/party etc in multistate.

If for whatever reason some of these actions need to be done from a map state script, then I suppose LuaVal could be used as some form of message queue which could be checked during the global state execution and handled accordingly without the need for any extra form of queueing.

[Shauren]

None of these are marked as world only

Eluna/TrinityCore/GroupMethods.h

Lines 451 to 457 in 6d3cdb7

	// Setters
	{ "SetLeader", &LuaGroup::SetLeader },
	{ "SetMembersGroup", &LuaGroup::SetMembersGroup },
	{ "SetTargetIcon", &LuaGroup::SetTargetIcon },
	#ifndef CATA
	{ "SetMemberFlag", &LuaGroup::SetMemberFlag },
	#endif

Eluna/TrinityCore/GroupMethods.h

Lines 476 to 477 in 6d3cdb7

	{ "ConvertToLFG", &LuaGroup::ConvertToLFG },
	{ "ConvertToRaid", &LuaGroup::ConvertToRaid },

Eluna/TrinityCore/GuildMethods.h

Lines 262 to 266 in 6d3cdb7

	{ "SetBankTabText", &LuaGuild::SetBankTabText },
	{ "SetMemberRank", &LuaGuild::SetMemberRank },
	#ifndef CATA
	{ "SetLeader", &LuaGuild::SetLeader },
	#endif

Eluna/TrinityCore/GuildMethods.h

Lines 271 to 277 in 6d3cdb7

	{ "Disband", &LuaGuild::Disband },
	{ "AddMember", &LuaGuild::AddMember },
	{ "DeleteMember", &LuaGuild::DeleteMember },
	#ifdef CATA //Not implemented in TCPP
	{ "SetLeader", nullptr, METHOD_REG_NONE },
	#endif

Eluna/TrinityCore/PlayerMethods.h

Lines 4077 to 4078 in 6d3cdb7

	{ "GroupInvite", &LuaPlayer::GroupInvite },
	{ "GroupCreate", &LuaPlayer::GroupCreate },

Eluna/TrinityCore/PlayerMethods.h

Lines 4050 to 4053 in 6d3cdb7

	{ "BindToInstance", &LuaPlayer::BindToInstance },
	{ "UnbindInstance", &LuaPlayer::UnbindInstance },
	{ "UnbindAllInstances", &LuaPlayer::UnbindAllInstances },
	{ "RemoveFromBattlegroundRaid", &LuaPlayer::RemoveFromBattlegroundRaid },

You can obtain a reference to guild/group from player methods

Eluna/TrinityCore/PlayerMethods.h

Lines 3811 to 3812 in 6d3cdb7

	{ "GetGroup", &LuaPlayer::GetGroup },
	{ "GetGuild", &LuaPlayer::GetGuild },

Eluna/TrinityCore/PlayerMethods.h

Line 3864 in 6d3cdb7

{ "GetOriginalGroup", &LuaPlayer::GetOriginalGroup },

[Rochet2]

all exposed functions mutating group and guild

Getting members of guild/group can also be unsafe depending on use. Tricky.

queue the code for later execution on world thread (queue itself must be threadsafe)

For executing functions from map states, communicating actions between states is one option.
However, that is not maybe the most efficient way to handle executing functions from map context.
It is also slightly limited as passing data between the states is limited and relatively slow. Having a mechanism for delayed execution of some code would make usage much more user friendly, but adds implementation complexity.

In my mind this would indeed mean that when some kind of function is called, a function of the current lua state is queued to be run on the world update thread later. We have had some discussion before on how we could achieve this.

Modify each function to be asynchronous, for example DoForPlayersInWorld(function (player) ...code... end) where a closure would run for all players on world update later.

Alternatively create a kind of privileged mode achieved in similar manner, which is delayed to world update, but allows one to synchronously call unsafe functions. This would be more generic and user friendly I feel.

Deferred(function(unsafe_functions)
    -- This code runs on world update
    -- We could have access to unsafe functions only through a parameter like unsafe_functions here
    local players = unsafe_functions:GetPlayersInWorld()
    -- Or we could have them as global functions, that only exist/work inside deferred function
    local players = GetPlayersInWorld()
end)

I myself feel like something along the lines of Deferred(function(unsafe_functions) is the best choice so there is just one asynchonous function to make things simple and unsafe_functions would make it clear what functions exist in which scope.

Some problems to solve/consider:

1. As seen above, these functions may allow access to references outside of current map. For example players on other maps. These references would need to be inaccessible after the asynchronous function call ends. This would warrant reintroduction of the invalidation system which was used for all events before. In it all objects were invalidated at end of an event, which in this case would be any reference pushed during the async function call when the function call ends)
2. Something that none of the above solves is unsafe methods related to some objects. For example, if a map state has access to group through local group = player:GetGroup() then how do we handle calling/blocking local members = group:GetMembers() when in and out of privilaged mode? Global functions are a bit easier to move into unsafe_functions, but unsafe member functions are less clear on how they could be handled. I somewhat dislike the idea of having functions that sometimes work and sometimes dont. But I feel like that is the only good option I can think of here. The member functions would print an error if not in privileged mode.

[Foereaper]

I think step one is marking the methods outlined above as world only for now, and then we can think of a cleaner way to either defer or queue inline functions to be executed post map update.

I'm not overly worried about GetGuild and GetGroup in the map thread, I feel these are probably valid to have available, though they would be limited in scope to what methods would work on said objects with a lot of the methods marked as world only.

[Shauren]

Oh I only pointed those out as a way to access a group/guild objects outside of their dedicated group/guild event hooks that run in world thread only

[Foereaper]

Also, the unbind/bind methods in player, are these not inherently safe in the map context since they are strictly tied to the player that already exists in the map context?

[Shauren]

Also, the unbind/bind methods in player, are these not inherently safe in the map context since they are strictly tied to the player that already exists in the map context?

Ignore it, bind/unbind are safe (I thought at first that the InstanceSave object isnt safe to use but it has a mutex)

As for deferring execution, I did not think that far, only imagined doing that purely in c++, for example having LuaGuild::AddMember be implemented as

int AddMember(Eluna* E, Guild* guild)
{
    Player* player = E->CHECKOBJ<Player>(2);
    uint8 rankId = E->CHECKVAL<uint8>(3, GUILD_RANK_NONE);

    sWorld->IMAGINARY_ASYNC_QUEUE_QUEUEING_FUNCTION([guildId = guild->GetId(), guid = player->GET_GUID(), rankId]()
    {
        if (Guild* guild = sGuildMgr->GetGuildById(guildId))
        {
            CharacterDatabaseTransaction trans(nullptr);
            guild->AddMember(trans, guid, rankId);
        }
    });
    return 0;
}

[Foereaper]

Pushed an update to flag the outlined methods above as world only, let me know if you find others and I'll flag those accordingly as well.

[Rochet2]

The approach in #474 (comment) would work fine for most if not all cases. The only difficulty is with the GetMembers of guild, group and the get players in world.

We could just keep the player object getter functions on world state.
Or they could return guids instead of objects and if the guild/group functions would operate on guids rather than objects it would allow doing actions on players that are not on the same map through the guids.

[Shauren]

The approach in #474 (comment) would work fine for most if not all cases. The only difficulty is with the GetMembers of guild, group and the get players in world.

We could just keep the player object getter functions on world state. Or they could return guids instead of objects and if the guild/group functions would operate on guids rather than objects it would allow doing actions on players that are not on the same map through the guids.

Latest version of #469 should actually handle that case, it prevents accessing objects whose bound map is different than current eluna state bound map