-
Notifications
You must be signed in to change notification settings - Fork 372
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
Multistate thread unsafety #474
Comments
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. |
None of these are marked as world only Eluna/TrinityCore/GroupMethods.h Lines 451 to 457 in 6d3cdb7
Eluna/TrinityCore/GroupMethods.h Lines 476 to 477 in 6d3cdb7
Eluna/TrinityCore/GuildMethods.h Lines 262 to 266 in 6d3cdb7
Eluna/TrinityCore/GuildMethods.h Lines 271 to 277 in 6d3cdb7
Eluna/TrinityCore/PlayerMethods.h Lines 4077 to 4078 in 6d3cdb7
Eluna/TrinityCore/PlayerMethods.h Lines 4050 to 4053 in 6d3cdb7
You can obtain a reference to guild/group from player methods Eluna/TrinityCore/PlayerMethods.h Lines 3811 to 3812 in 6d3cdb7
Eluna/TrinityCore/PlayerMethods.h Line 3864 in 6d3cdb7
|
Getting members of guild/group can also be unsafe depending on use. Tricky.
For executing functions from map states, communicating actions between states is one option. 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 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 Some problems to solve/consider:
|
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. |
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 |
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
|
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. |
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. |
Latest version of #469 should actually handle that case, it prevents accessing objects whose bound map is different than current eluna state bound map |
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:
Solution could be one of
The text was updated successfully, but these errors were encountered: