[ani]

eMule 0.46c (good)

void CSearchManager::jumpStart(void)
{
	time_t now = time(NULL);
	try
	{
  SearchMap::iterator it = m_searches.begin(); 
  while (it != m_searches.end())
  {
  	switch(it->second->getSearchTypes()){
    case CSearch::FILE:
    {

eMule 0.47a (bad)

void CSearchManager::JumpStart()
{
	// Find any searches that has stalled and jumpstart them.
	// This will also prune all searches.
	time_t tNow = time(NULL);
	for (SearchMap::iterator itSearchMap = m_mapSearches.begin(); itSearchMap != m_mapSearches.end(); [B]++itSearchMap[/B])
	{
  // Each type has it's own criteria for being deleted or jumpstarted.
  switch(itSearchMap->second->GetSearchTypes())
  {

It's obvious that the author of 0.47a does not understand the nuances of C++ STL. You will see some iterators get incremented twice. Deleting any items in any container invalidates the iterator. Microsoft implements something non-standard which allows you to get an iterator to the next item after the deleted iterator. Rather than expounding on Microsoft's way, this is a programming bug that's causing Kademlia to crash eMule.

This post has been edited by ani: 09 April 2006 - 03:19 PM

[SiRoB]

Hum read again and i'm not sur.
Is ".erase(" return the next or the previous?

[tHeWiZaRdOfDoS]

MSDN:

Quote

Removes an element or a range of elements in a map from specified positions or removes elements that match a specified key.

iterator erase(
  iterator _Where
);
iterator erase(
  iterator _First,
  iterator _Last
);
size_type erase(
  const key_type& _Key
);
Parameters
_Where
Position of the element to be removed from the map.
_First
Position of the first element removed from the map.
_Last
Position just beyond the last element removed from the map.
_Key
The key value of the elements to be removed from the map.
Return Value
For the first two member functions, a bidirectional iterator that designates the first element remaining beyond any elements removed, or a pointer to the end of the map if no such element exists.

For the third member function, returns the number of elements that have been removed from the map.
...

That means it might skip entries but no crash should/could occur.

[SiRoB]

if we are too lazy to put back the numerous iterator incrementation from 0.46c code, just make this:

Quote

itSearchMap = --m_mapSearches.erase(itSearchMap);

[tHeWiZaRdOfDoS]

Just a small note I think

Quote

It's obvious that the author of 0.47a does not understand the nuances of C++ STL.

is a very insolent way to point something out!

[niclights]

Phew. I'm glad someone said that! IIRC the original post was even more aggressive. If someone finds a potential fault, then great, but to say it that way was shocking.

[leuk_he]

ani, on Apr 9 2006, 04:00 PM, said:

this is a programming bug that's causing Kademlia to crash eMule.

If it would be a bug (which i doubt....) it would only print ugly logline about an exception in the log. Is that is your definition of a crash....

[tHeWiZaRdOfDoS]

BTW....

Quote

void CSearchManager::UpdateStats()
{
CPrefs* pPrefs = CKademlia::GetPrefs();
if(!pPrefs)
  return;


// Update stats on the searches, this info can be used to determine if we need can start new searches.
uint8 uTotalFile = 0;
uint8 uTotalStoreSrc = 0;
uint8 uTotalStoreKey = 0;
uint8 uTotalSource = 0;
uint8 uTotalNotes = 0;
uint8 uTotalStoreNotes = 0;
for (SearchMap::const_iterator itSearchMap = m_mapSearches.begin(); itSearchMap != m_mapSearches.end(); ++itSearchMap)
{
  switch(itSearchMap->second->GetSearchTypes())
  {
  case CSearch::FILE:
    uTotalFile++;
    break;
  case CSearch::STOREFILE:
    uTotalStoreSrc++;
    break;
  case CSearch::STOREKEYWORD:
    uTotalStoreKey++;
    break;
  case CSearch::FINDSOURCE:
    uTotalSource++;
    break;
  case CSearch::STORENOTES:
    uTotalStoreNotes++;
    break;
  case CSearch::NOTES:
    uTotalNotes++;
    break;
  }
}

/* CPrefs *pPrefs = CKademlia::GetPrefs();
if(pPrefs)*/
{
  pPrefs->SetTotalFile(uTotalFile);
  pPrefs->SetTotalStoreSrc(uTotalStoreSrc);
  pPrefs->SetTotalStoreKey(uTotalStoreKey);
  pPrefs->SetTotalSource(uTotalSource);
  pPrefs->SetTotalNotes(uTotalNotes);
  pPrefs->SetTotalStoreNotes(uTotalStoreNotes);
}
}

Might save a few cycles
Never checked out the KAD-Source in detail, maybe we should pay more attention to it

[leuk_he]

tHeWiZaRdOfDoS, on Apr 10 2006, 09:53 AM, said:

Might save a few cycles
Never checked out the KAD-Source in detail, maybe we should pay more attention to it

This part is not really motivating to look at it:

(kademidla.cpp...)

Quote

// Note To Mods //
/*
Please do not change anything here and release it..
There is going to be a new forum created just for the Kademlia side of the client..
If you feel there is an error or a way to improve something, please
post it in the forum first and let us look at it.. If it is a real improvement,
it will be added to the offical client.. Changing something without knowing
what all it does can cause great harm to the network if released in mass form..
Any mod that changes anything within the Kademlia side will not be allowed to advertise
there client on the eMule forum..
*/

however "unknow" has been known to reply in the kad subforum. why not post your optimization there?

This post has been edited by leuk_he: 10 April 2006 - 10:59 AM

[drolevar]

I can confirm that this issue leads to a crash in case the code is compiled with VC 8.0. Also the same issue is in the CSearchManager::StopSearch method.

[boohb]

Crashing when compiling with VC 8.0 for me as well...
does anyone know of a fix?

Thanks,
booh

[Unknown1]

Well, bugs are called bugs for a reason.. Nobody is perfect.. People that say statements like the author of this thread usually have issues somewhere else in their life and vent them in this manor.. Either way, the bug will be fixed for next release.. I'll post my patch soon..

FYI: Like said earlier.. The crash is not for the reason the author says as the erase dose not invalidate my iterator.. It's because the for-loop will try to get past the end..

drolevar, on Apr 10 2006, 05:57 AM, said:

I can confirm that this issue leads to a crash in case the code is compiled with VC 8.0. Also the same issue is in the CSearchManager::StopSearch method.

There is a return in there for StopSearch.. Nothing is executated after the erase.. But I do need to make a comment in there just in case this method is modified at a later date so the same bug doesn't show up again.

Well, sure some will flame the patch. But here it is..

	// Find any searches that has stalled and jumpstart them.
	// This will also prune all searches.
	time_t tNow = time(NULL);
	SearchMap::iterator itSearchMap = m_mapSearches.begin();
	while(itSearchMap != m_mapSearches.end())
	{
  // Each type has it's own criteria for being deleted or jumpstarted.
  switch(itSearchMap->second->GetSearchTypes())
  {
  	case CSearch::FILE:
    {
    	if (itSearchMap->second->m_tCreated + SEARCHFILE_LIFETIME < tNow)
    	{
      delete itSearchMap->second;
      itSearchMap = m_mapSearches.erase(itSearchMap);
      //Don't do anything after this.. We are already at the next entry.
      continue;
    	}
...
  }
  ++itSearchMap;
	}

Basically put a continue after each "erase" on all code I ommited and it should be ok..