GetDotaStats / stat-collection

Library for setting up stat collection for dota2 mods
http://getdotastats.com/
GNU General Public License v2.0
11 stars 8 forks source link

Stats causing server crashes #60

Closed jimmydorry closed 8 years ago

jimmydorry commented 8 years ago

http://i.imgur.com/gWj2tno.png

Not enough data or reports to do much with, yet.

Ebf and legiontd affected when using stats, legiontd dropped stats which fixed it. May not be convars.

EDIT: Was likely the PlayerResource:GetSteamAccountID(hostID)

jimmydorry commented 8 years ago

Response from Valve. http://i.imgur.com/rHVLtzg.png

Doesn't make much sense to me, as I can't see anywhere we pass a playerID into GetSteamAccountID().

How ever, perhaps the error is when we evaluate the GetPlayerCount() at the start? Perhaps this evaluates to an exception on dedi's?

https://github.com/GetDotaStats/stat-collection/blob/master/scripts/vscripts/statcollection/init.lua#L16

jimmydorry commented 8 years ago

As @SinZ163 pointed out, it's probably hostID remaining as nil.

https://github.com/GetDotaStats/stat-collection/blob/master/scripts/vscripts/statcollection/lib/statcollection.lua#L258

It's weird that they said the error is PlayerResource:GetSteamAccountID(playerID), as that would indicate it not being this line... but we should fix that edge case regardless.

jimmydorry commented 8 years ago

While we are at it, the following line seems inconsistent with what we normally do (a loop using DOTA_MAX_PLAYERS verified with PlayerResource:IsValidPlayerID()

https://github.com/GetDotaStats/stat-collection/blob/master/scripts/vscripts/statcollection/lib/statcollection.lua#L392

It probably should be made more like:

https://github.com/GetDotaStats/stat-collection/blob/master/scripts/vscripts/statcollection/lib/statcollection.lua#L330

jimmydorry commented 8 years ago

Here is a pretty ham-fisted approach. I'm sure you guys can do a much better job, or we may even decide that knowing the host is something we can do without for dedicated servers.

https://github.com/GetDotaStats/stat-collection/compare/a6a6d432cfed...7e485c0ba7cb

https://github.com/GetDotaStats/stat-collection/tree/v3.1

jimmydorry commented 8 years ago

Modders notified of situation and fix, now that it has been confirmed.

Hi guys,

In a combo of poor code on Valve's end and a silly bug on our end, the v3 of the stat-collection library causes dedicated servers to crash. Valve will likely roll an update to fix this soon. In the mean-time, if your mod has a dedicated server, disable stats temporarily, by commenting out the line in your addon_game_mode.lua that initiates the library:

--require('statcollection/init')

This only affects mods running v3 of the library, that have dedicated enabled on their mod.

We have a v3.1 that probably fixes the issue, but we need to have it tested first. If you want to give it a spin, that would be appreciated (as your mod is likely broken anyway).

https://github.com/GetDotaStats/stat-collection/tree/v3.1

For the technically inclined, you can crash a dedicated server by calling the following function with a value of nil.

PlayerResource:GetSteamAccountID(nil)

The GetDotaStats team is really sorry about how this has affected many of your mod's rankings, and we are immensely disappointed that such a silly bug could cause so much harm.

If you would like to help make sure this kind of thing never happens again, please send an email to Valve asking for them to facilitate a 'dedicated server test-box', where modders with a dedicated-server-enabled mod can test their mod before deploying. You can do this by using their form, and selecting Dota 2 Team in the TO: field.

Regards, jimmydorry On behalf of the GetDotaStats team

jimmydorry commented 8 years ago

Need to review the fix, and test to see if that fixes it.

https://github.com/GetDotaStats/stat-collection/pull/61

jimmydorry commented 8 years ago

Response from Valve regarding v3

The update we published this evening fixes the problem we were hitting in the Lua interpreter. We've tested the previous version of Epic Boss Fight on our internal dedicated servers and it now works correctly with this build. If you could update Epic Boss Fight to enable your stats collection again and give it a quick test, it would be great to hear confirmation if we've fixed the problem.

MNoya commented 8 years ago

Edited format with >quote