fix Some Problems With ShopEX(Memory Leak, open_shop)

[Mali 39723]

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

[Kafa 148]

Interesting 

Edited April 21, 2020 by Kafa

[Sherer 459]

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

[Mali 39723]

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

[xP3NG3Rx 19121]

There will be one, who knows better everything, always. Always, no matter what.

 

[Sherer 459]

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

[Mali 39723]

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

[Sherer 459]

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

[Hik 105]

Could you tell me how you configured the projects to get a debug like this?