azerothcore-wotlk icon indicating copy to clipboard operation
azerothcore-wotlk copied to clipboard

Published 20 hours ago • verified publisher icon azerothcore

fix(Battlefield/SpawnCreature) Using unique_ptr

Open pangolp opened this issue 1 year ago • 6 comments

Changes Proposed:

This PR proposes changes to:

  • [x] Core (units, players, creatures, game systems).
  • [x] Scripts (bosses, spell scripts, creature scripts).

Issues Addressed:

  • Closes

SOURCE:

The changes have been validated through:

==8664== Memcheck, a memory error detector
==8664== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==8664== Using Valgrind-3.16.1 and LibVEX; rerun with -h for copyright info
==8664== Command: ./authserver
==8664==
AzerothCore rev. d71f5dcfb2c4 2023-07-18 16:51:16 -0300 (master branch) (Unix, RelWithDebInfo, Static) (authserver)
<Ctrl-C> to stop.

   █████╗ ███████╗███████╗██████╗  ██████╗ ████████╗██╗  ██╗
  ██╔══██╗╚══███╔╝██╔════╝██╔══██╗██╔═══██╗╚══██╔══╝██║  ██║
  ███████║  ███╔╝ █████╗  ██████╔╝██║   ██║   ██║   ███████║
  ██╔══██║ ███╔╝  ██╔══╝  ██╔══██╗██║   ██║   ██║   ██╔══██║
  ██║  ██║███████╗███████╗██║  ██║╚██████╔╝   ██║   ██║  ██║
  ╚═╝  ╚═╝╚══════╝╚══════╝╚═╝  ╚═╝ ╚═════╝    ╚═╝   ╚═╝  ╚═╝
                                 ██████╗ ██████╗ ██████╗ ███████╗
                                ██╔════╝██╔═══██╗██╔══██╗██╔════╝
                                ██║     ██║   ██║██████╔╝█████╗
                                ██║     ██║   ██║██╔══██╗██╔══╝
                                ╚██████╗╚██████╔╝██║  ██║███████╗
                                 ╚═════╝ ╚═════╝ ╚═╝  ╚═╝╚══════╝

     AzerothCore 3.3.5a  -  www.azerothcore.org

> Using configuration file       /home/wow/azeroth-server/etc/authserver.conf
> Using SSL version:             OpenSSL 1.1.1n  15 Mar 2022 (library: OpenSSL 1.1.1n  15 Mar 2022)
> Using Boost version:           1.74.0

Opening DatabasePool 'argwow_auth'. Asynchronous connections: 1, synchronous connections: 1.
MySQL client library: 10.5.19
MySQL server ver: 10.5.19-MariaDB-0+deb11u2
Connected to MySQL database at 127.0.0.1
MySQL client library: 10.5.19
MySQL server ver: 10.5.19-MariaDB-0+deb11u2
Connected to MySQL database at 127.0.0.1
DatabasePool 'argwow_auth' opened successfully. 2 total connections running.

Updating Auth database...
>> Auth database is up-to-date! Containing 1 new and 38 archived updates.

Started auth database connection pool.
Loading IP Location Database...

Added realm "Andes WOW" at 200.73.128.21:24512.
^CHalting process...
Closing down DatabasePool 'argwow_auth'.
Asynchronous connections on DatabasePool 'argwow_auth' terminated. Proceeding with synchronous connections.
All connections on DatabasePool 'argwow_auth' closed.
==8664==
==8664== HEAP SUMMARY:
==8664==     in use at exit: 0 bytes in 0 blocks
==8664==   total heap usage: 9,239 allocs, 9,239 frees, 3,054,315 bytes allocated
==8664==
==8664== All heap blocks were freed -- no leaks are possible
==8664==
==8664== For lists of detected and suppressed errors, rerun with: -s
==8664== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
==8674==
==8674== LEAK SUMMARY:
==8674==    definitely lost: 1,681,176 bytes in 182 blocks
==8674==    indirectly lost: 505,063 bytes in 1,368 blocks
==8674==      possibly lost: 0 bytes in 0 blocks
==8674==    still reachable: 68,987,279 bytes in 307 blocks
==8674==                       of which reachable via heuristic:
==8674==                         length64           : 55,104 bytes in 32 blocks
==8674==         suppressed: 0 bytes in 0 blocks
==8674== Reachable blocks (those to which a pointer was found) are not shown.
==8674== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==8674==
==8674== For lists of detected and suppressed errors, rerun with: -s
==8674== ERROR SUMMARY: 58 errors from 19 contexts (suppressed: 0 from 0)
==8674== 172,480 (157,472 direct, 15,008 indirect) bytes in 14 blocks are definitely lost in loss record 239 of 245
==8674==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==8674==    by 0xA36013: Battlefield::SpawnCreature(unsigned int, float, float, float, float, TeamId) (src/server/game/Battlefield/Battlefield.cpp:796)
==8674==    by 0xA41472: BfWGGameObjectBuilding::Init(GameObject*, unsigned int, unsigned int, unsigned char, unsigned char) (src/server/game/Battlefield/Zones/BattlefieldWG.h:1327)
==8674==    by 0xA3D5A9: BattlefieldWG::SetupBattlefield() (src/server/game/Battlefield/Zones/BattlefieldWG.cpp:173)
==8674==    by 0xA3AFCC: BattlefieldMgr::InitBattlefield() (src/server/game/Battlefield/BattlefieldMgr.cpp:51)
==8674==    by 0x10F0C99: World::SetInitialWorldSettings() (src/server/game/World/World.cpp:2107)
==8674==    by 0x43CAF8: main (src/server/apps/worldserver/Main.cpp:308)
==8674==
==8674== 202,752 (179,968 direct, 22,784 indirect) bytes in 16 blocks are definitely lost in loss record 240 of 245
==8674==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==8674==    by 0xA36013: Battlefield::SpawnCreature(unsigned int, float, float, float, float, TeamId) (src/server/game/Battlefield/Battlefield.cpp:796)
==8674==    by 0xA3D354: BattlefieldWG::SetupBattlefield() (src/server/game/Battlefield/Zones/BattlefieldWG.cpp:161)
==8674==    by 0xA3AFCC: BattlefieldMgr::InitBattlefield() (src/server/game/Battlefield/BattlefieldMgr.cpp:51)
==8674==    by 0x10F0C99: World::SetInitialWorldSettings() (src/server/game/World/World.cpp:2107)
==8674==    by 0x43CAF8: main (src/server/apps/worldserver/Main.cpp:308)
==8674==
==8674== 469,041 (427,424 direct, 41,617 indirect) bytes in 38 blocks are definitely lost in loss record 241 of 245
==8674==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==8674==    by 0xA36013: Battlefield::SpawnCreature(unsigned int, float, float, float, float, TeamId) (src/server/game/Battlefield/Battlefield.cpp:796)
==8674==    by 0xA3D104: BattlefieldWG::SetupBattlefield() (src/server/game/Battlefield/Zones/BattlefieldWG.cpp:130)
==8674==    by 0xA3AFCC: BattlefieldMgr::InitBattlefield() (src/server/game/Battlefield/BattlefieldMgr.cpp:51)
==8674==    by 0x10F0C99: World::SetInitialWorldSettings() (src/server/game/World/World.cpp:2107)
==8674==    by 0x43CAF8: main (src/server/apps/worldserver/Main.cpp:308)
==8674==
==8674== 480,418 (438,672 direct, 41,746 indirect) bytes in 39 blocks are definitely lost in loss record 242 of 245
==8674==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==8674==    by 0xA36013: Battlefield::SpawnCreature(unsigned int, float, float, float, float, TeamId) (src/server/game/Battlefield/Battlefield.cpp:796)
==8674==    by 0xA3D157: BattlefieldWG::SetupBattlefield() (src/server/game/Battlefield/Zones/BattlefieldWG.cpp:133)
==8674==    by 0xA3AFCC: BattlefieldMgr::InitBattlefield() (src/server/game/Battlefield/BattlefieldMgr.cpp:51)
==8674==    by 0x10F0C99: World::SetInitialWorldSettings() (src/server/game/World/World.cpp:2107)
==8674==    by 0x43CAF8: main (src/server/apps/worldserver/Main.cpp:308)

Tests Performed:

How to Test the Changes:

  1. When using the Valgrind tool, it was detected that in the method, the memory was not being freed after its use. In turn, a try is applied to capture the error if one exists.
  2. To test this pull request, you should use the tool, reanalyze and determine if with this change, nothing is no longer mentioned regarding this method.

Known Issues and TODO List:

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

pangolp avatar Feb 16 '24 02:02 pangolp

Need people to go in a bg with this PR to see if creatures still spawn. They probably will but just in case. I can test tonight as well

elthehablo avatar Feb 16 '24 12:02 elthehablo

Need people to go in a bg with this PR to see if creatures still spawn. They probably will but just in case. I can test tonight as well

Yes, no problem. It is a delicate pull request to test. I wasn't even sure how to do it. But if it works, we could solve several leaks, although there are more, but I would treat them separately and not all together in case any of them had to be reversed. If you give me the steps to follow, I'll try it locally too.

pangolp avatar Feb 16 '24 12:02 pangolp

I would have to see if I can compile the branch in Debian 12, use valgrind and that way, see if the messages about this method disappear. Beyond the fact that there seems to be more, as I mentioned before, it would be good to attack them separately, in case any of them had to be reversed. Although also, we have to do tests within the game, as you mentioned @elthehablo

pangolp avatar Feb 16 '24 15:02 pangolp

We don't use try-catches in the core as they are expensive and code should be made not to go wrong in the first place

Nyeriah avatar Feb 17 '24 12:02 Nyeriah

Perfect, I'm trying to see if this resolves the memory leaks, using valgrind again because if that didn't happen, at least in that method, perhaps it wouldn't make sense to use it. If it works, rewrite the code, without using the try-catch.

pangolp avatar Feb 17 '24 13:02 pangolp

Unfortunately, the problem persists, it's just been moved offline now. So I will have to continue investigating a little more, and I will take the opportunity to eliminate the try-catch block.

==1515== 472,377 (404,928 direct, 67,449 indirect) bytes in 36 blocks are definitely lost in loss record 283 of 286
==1515==    at 0x4838DEF: operator new(unsigned long) (vg_replace_malloc.c:342)
==1515==    by 0xAA3DD3: make_unique<Creature, bool> (unique_ptr.h:962)
==1515==    by 0xAA3DD3: Battlefield::SpawnCreature(unsigned int, float, float, float, float, TeamId) (src/server/game/Battlefield/Battlefield.cpp:804)
==1515==    by 0xAAB124: BattlefieldWG::SetupBattlefield() (src/server/game/Battlefield/Zones/BattlefieldWG.cpp:130)
==1515==    by 0xAA8FEC: BattlefieldMgr::InitBattlefield() (src/server/game/Battlefield/BattlefieldMgr.cpp:51)
==1515==    by 0x11505E5: World::SetInitialWorldSettings() (src/server/game/World/World.cpp:2103)
==1515==    by 0x43D435: main (src/server/apps/worldserver/Main.cpp:308)
==1515==
==1515== LEAK SUMMARY:
==1515==    definitely lost: 1,568,544 bytes in 176 blocks
==1515==    indirectly lost: 753,423 bytes in 1,896 blocks
==1515==      possibly lost: 0 bytes in 0 blocks
==1515==    still reachable: 69,001,152 bytes in 310 blocks
==1515==                       of which reachable via heuristic:
==1515==                         length64           : 68,936 bytes in 32 blocks
==1515==         suppressed: 0 bytes in 0 blocks
==1515== Reachable blocks (those to which a pointer was found) are not shown.
==1515== To see them, rerun with: --leak-check=full --show-leak-kinds=all
==1515==
==1515== For lists of detected and suppressed errors, rerun with: -s
==1515== ERROR SUMMARY: 24 errors from 24 contexts (suppressed: 0 from 0)

pangolp avatar Feb 17 '24 15:02 pangolp

Unfortunately, this change, beyond the fact that modifications had to be made, does not serve to eliminate the memory leak, because the problem is a little deeper. Within creature classes, and their corresponding parents, memory purging is not performed, so pointers of this type cannot be used. Most classes use override, even though they do not overwrite anything, and I don't know if, in part, the problem is also there. So it would be better to close this pull request, and continue investigating further.

pangolp avatar Feb 19 '24 00:02 pangolp