World crash, ProcessCombatMovement()

[John Adams]

The player that was online wasn't even in combat, so I suspect this is NPC/faction combat again.

Code: Select all

>	EQ2World.exe!NPC_AI::ProcessCombatMovement()  Line 156 + 0x10 bytes	C++
 	EQ2World.exe!ZoneServer::SpawnProcess()  Line 1005	C++
 	EQ2World.exe!SpawnLoop(void * tmp)  Line 4617 + 0x5 bytes	C++
 	EQ2World.exe!_callthreadstart()  Line 259 + 0x6 bytes	C
 	EQ2World.exe!_threadstart(void * ptd)  Line 241 + 0x5 bytes	C
 	kernel32.dll!77e6481f() 	
 	[Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]	

target_location was null even though it gets checked a few lines up (we've seen this before)


Attempting this as a fix:

Code: Select all

		// Get our distance from the target location
		distance_from_target = npc->GetDistance(target_location->x, target_location->y, target_location->z, true);
-		if (target)
+		if (target && target_location)
			target_distance_from_target = target->GetDistance(target_location->x, target_location->y, target_location->z, true);
+		else
+			target_distance_from_target = 0;

Reply, tell me if this is as good a guess as any.


I'd like crashes to take top priority over all other tasks. We've had a very stable server for a long time, we do not need to lose traction now.

[Jabantiz]

I swear we had this problem before and fixed it...

I will have to take another look at movement and combat as adding features to either is getting ugly as hell in my opinion.

[John Adams]

I agree. Though I am not looking for a revamp (again hah) but over time, maybe we can simplify some stuff. As for this happening before, I am positive too, but couldn't find the post. I believe I fixed it this same way on EQ2TC local code, but maybe I didn't commit it waiting to verify it was a proper fix.

I have committed it this time though, so if you think it improper, please make adjustments.

[Jabantiz]

What bothers me about this issue is just a few lines up we check that pointer and if it is not valid it skips to the next element in the list

Code: Select all

MovementLocation* target_location = npc->GetLastRunningLocation();
		if (!target_location) {

I have been messing around with an idea that will combine all movement code and make all this combat movement code obsolete, I have been getting some results but it still needs work and it reintroduced the warping issue, though I am not sure how or why.

[Jabantiz]

committed some movement changes, this combines the normal movement and combat movement together and combat movement was simplified. So far this seems to be better and more stable then what we currently have. I tested this some and thefoof jumped on my server and really tested it out. There was one instance of lag but not sure it was related to the movement so need to keep an eye on that.

[John Adams]

Using new movement code, a player was online at 23:53 last night, and abruptly was removed. Queens Colony zone is dead. No connections taken to it since. I just logged in to Darklight fine, but as soon as I /zone QueensColony, my character is also now stuck - no communication.

It's like that old bug where if, for any reason, you do not connect fully, the zone is dead til the world is reset. I do not know how to give you any more info than that. Not a crash, just a lock up it seems.

If I click Pause on debug, I can step into code, but it seems to only be stuck here in this while -

EQStreamFactory.cpp -

Code: Select all

	while(sock!=-1) {
		Timer::SetCurrentTime();
		//if (!havework) {
			//WriterWork.Wait();
		//}
		MWriterRunning.lock();
		if (!WriterRunning)
			break;
		MWriterRunning.unlock();
		
		wants_write.clear();

		decay=DecayTimer.Check();
		
		//copy streams into a seperate list so we dont have to keep
		//MStreams locked while we are writting
		MStreams.lock();
		for(stream_itr=Streams.begin();stream_itr!=Streams.end();stream_itr++) {
			// If it's time to decay the bytes sent, then let's do it before we try to write
			if(!stream_itr->second){
				Streams.erase(stream_itr);
				break;
			}
			if (decay)
				stream_itr->second->Decay();

			if (stream_itr->second->HasOutgoingData()) {
				stream_itr->second->PutInUse();
				wants_write.push_back(stream_itr->second);
			}
			if(stream_itr->second->resend_que_timer->Check())
				resend_que.push_back(stream_itr->second);
		}
		MStreams.unlock();
		
		//do the actual writes
		cur = wants_write.begin();
		end = wants_write.end();
		for(; cur != end; cur++) {
			(*cur)->Write(sock);
			(*cur)->ReleaseFromUse();
		}
		while(resend_que.size()){
			resend_que.front()->CheckResend(sock);
			resend_que.pop_front();
		}
		Sleep(10);

		MStreams.lock();
		stream_count=Streams.size();
		MStreams.unlock();
		if (!stream_count) {
			//cout << "No streams, waiting on condition" << endl;
			WriterWork.Wait();
			//cout << "Awake from condition, must have a stream now" << endl;
		}
	}

[Jabantiz]

I am not familiar with this code not sure where it runs but I did notice this

Code: Select all

MWriterRunning.lock();
      if (!WriterRunning)
         break;
      MWriterRunning.unlock();

That break will kill the loop it is in but leave the lock in place so the next time something tries to lock it there will be a deadlock, not sure that is the issue though would have to study the code and try to figure out what is going wrong.

I tested this in ruins and while on initial startup still had lag issues it wasn't as bad as it was, thefoof tested on my server in queens colony and was getting a lot of agro and running them around and all we noticed was some lag that may or may not have been related to the changes (I had a lot of other stuff running on my comp at the time on a debug build with a debugger attached to it). I will try to do more extensive tests to reproduce the deadlock.

[John Adams]

Jab, do your combat/movement issues side-step this potential deadlock? Trying to close bug posts today.

[Jabantiz]

Yes it should as NPC_AI::ProcessCombatMovement() no longer exists and the method I use now is completely different and doesn't rely on pointers to locations anymore.