Honorable Member Mali 42429 Posted April 19, 2020 Honorable Member Share Posted April 19, 2020 (edited) I found such a problem while testing the npc.open_shop(vnum) function at shopex. I debugged and i found a memory leak. https://metin2.download/picture/r3F9504x09DVmF8K988YXgTtGTP6KMUj/.gif Spoiler Test: 1 normal shop, 2 shopex (9001,9002,9007) m_map_pkShopByNPCVnum is ok, size is 3 (9001,9002,9007) m_map_pkShop size is 2 normal shops adding with no problem, but shopex objects adding only 1 time, example if you have 10 shopex only adding 1 object. And this is destructor: (only m_map_pkShop is cleaning, so we have memory leak for other shopex objects) : shop_manager.cpp: //Find if (pkShopEx->GetVnum() != 0 && m_map_pkShop.find(pkShopEx->GetVnum()) != m_map_pkShop.end()) { sys_err("Shop vnum(%d) already exist.", pkShopEx->GetVnum()); return false; } m_map_pkShop.insert(TShopMap::value_type(pkShopEx->GetVnum(), pkShopEx)); ///Change if (m_map_pkShop.find(table.dwVnum) != m_map_pkShop.end()) { sys_err("Shop vnum(%d) already exist.", table.dwVnum); return false; } m_map_pkShop.insert(TShopMap::value_type(table.dwVnum, pkShopEx)); //Find void CShopManager::Destroy() { TShopMap::iterator it = m_map_pkShop.begin(); while (it != m_map_pkShop.end()) { delete it->second; ++it; } m_map_pkShop.clear(); } ///Change void CShopManager::Destroy() { for (auto it = m_map_pkShopByNPCVnum.begin(); it != m_map_pkShopByNPCVnum.end(); ++it) delete it->second; m_map_pkShopByNPCVnum.clear(); m_map_pkShop.clear(); } there is always a better method. This was the easiest solution that came to my mind. Edited August 27, 2022 by Metin2 Dev Core X - External 2 Internal 12 3 19 Link to comment Share on other sites More sharing options...
Kafa 153 Posted April 19, 2020 Share Posted April 19, 2020 (edited) Interesting Edited April 21, 2020 by Kafa Link to comment Share on other sites More sharing options...
Sherer 486 Posted April 20, 2020 Share Posted April 20, 2020 (edited) Just wrap it into smart pointer mate. Or... Clear data from m_map_pkShopByNPCVnum instead (since every, singleobject ends up there). Edited April 20, 2020 by Sherer 1 Link to comment Share on other sites More sharing options...
Honorable Member Mali 42429 Posted April 20, 2020 Author Honorable Member Share Posted April 20, 2020 (edited) 3 hours ago, Sherer said: Just wrap it into smart pointer mate. Or... Clear data from m_map_pkShopByNPCVnum instead (since every, singleobject ends up there). As I said there are other ways Edited April 20, 2020 by Mali61 misunderstanding, he was just talking about the delete part Link to comment Share on other sites More sharing options...
Honorable Member xP3NG3Rx 19764 Posted April 20, 2020 Honorable Member Share Posted April 20, 2020 There will be one, who knows better everything, always. Always, no matter what. 1 2 Link to comment Share on other sites More sharing options...
Sherer 486 Posted April 20, 2020 Share Posted April 20, 2020 Well, the collector you came up with doesn't really does the job for me (pkShopEx is not stored anyway in m_map_pkShop since insert fails in that case). That's why I mentioned it 1 Link to comment Share on other sites More sharing options...
Honorable Member Mali 42429 Posted April 20, 2020 Author Honorable Member Share Posted April 20, 2020 (edited) 22 minutes ago, Sherer said: Well, the collector you came up with doesn't really does the job for me (pkShopEx is not stored anyway in m_map_pkShop since insert fails in that case). That's why I mentioned it are you talking about ReadShopTableEx function fail? If an error occurs, the server will not start anyway. but ok it's not problem for me, i edited topic for m_map_pkShopByNPCVnum Edited August 25, 2022 by Metin2 Dev Core X - External 2 Internal Link to comment Share on other sites More sharing options...
Sherer 486 Posted April 20, 2020 Share Posted April 20, 2020 (edited) 43 minutes ago, Mali61 said: are you talking about ReadShopTableEx function fail? If an error occurs, the server will not start anyway. but ok it's not problem for me, i edited topic for m_map_pkShopByNPCVnum I mean you'd suggested to change part of m_map_pkShop destruction (introduced collector etc.). The problem is leak is not related to this container. Have a look at piece of code you provided: m_map_pkShop.insert According to C++ docs insert method tries to add element if it not exists. Because of that fact pkShopEx is never, ever added to map (if one exists, which you rightly mentioned). Bear in mind that clearing m_map_pkShopByNPCVnum is not enough since it stores pointers to objects (allocated with new operators). Your recent alterations already handle it well. Good job man. for (auto it = m_map_pkShopByNPCVnum.begin(); it != m_map_pkShopByNPCVnum.end(); ++it) delete it->second; Edited August 25, 2022 by Metin2 Dev Core X - External 2 Internal Link to comment Share on other sites More sharing options...
Hik 108 Posted April 20, 2020 Share Posted April 20, 2020 Could you tell me how you configured the projects to get a debug like this? Link to comment Share on other sites More sharing options...
Recommended Posts