[Server Crash] - Entityscript 908 crash

[rezecib]

Bug Submission:

Category: Server Crash
Issue Title: Entityscript 908 crash
Issue Description: I get this periodically, without any specific repro conditions. The game is checking the distance between two things, often a spider checking if a target is close enough to attack, I think, and crashes because the thing "isn't valid". Currently the code uses an assert to determine this, but because this seems to be something that can happen, but rarely happens, wouldn't it be better to just handle the error rather than crashing with it?

Here's the relevant code:

function EntityScript:GetDistanceSqToInst(inst)    assert(self:IsValid())    assert(inst:IsValid())    local p1x, p1y, p1z = self.Transform:GetWorldPosition()    local p2x, p2y, p2z = inst.Transform:GetWorldPosition()          --assert(p1x and p1z, "Something is wrong: self.Transform:GetWorldPosition() stale component reference?")    --assert(p2x and p2z, "Something is wrong: inst.Transform:GetWorldPosition() stale component reference?")          if p1x and p1z and p2x and p2x then         return distsq(p1x, p1z, p2x, p2z)    else        return 99999999.9 --Some large number    endend

Personally I've edited it to have the following above the asserts:

	if not (self:IsValid() and inst:IsValid()) then return 99999999.9 end 

Steps to Reproduce: The most reliable method is to just have a large server with a lot of stuff going on (e.g. lots of spider dens and pig houses for them to fight). Probably setting most things to Lots will get it done (but not Walrus Camps, those still have their own associated follower crash).

[bizziboi]

No, I'd really rather not patch the code at this level. The assert is there to indicate that something happened that shouldn't happen and that the issue should be fixed at a higher level or worse issues may happen.

 

What if the code was called with

 

if self:GetDistanceSqToInst(otherinst) > 100 then

   otherinst:DoWhatEver()

end

 

Then DoWhatEver would also need to be patched with IsValid and in the end IsValid is all over the place.

 

The fact that the distance function is called with an invalid entity points to an issue higher up, and patching the low level will only hide the problem, making resulting bugs even harder to find.

[rezecib]

@bizziboi I definitely understand that. I defaulted to a large distance because that's what it's doing lower-down, but I agree that if it can be fixed at the higher level, that's definitely preferable. As a side-note, it looks like the last if-then should have p2z instead of p2x at the end.

[kraken121]

No, I'd really rather not patch the code at this level. The assert is there to indicate that something happened that shouldn't happen and that the issue should be fixed at a higher level or worse issues may happen.

 

What if the code was called with

 

if self:GetDistanceSqToInst(otherinst) > 100 then

   otherinst:DoWhatEver()

end

 

Then DoWhatEver would also need to be patched with IsValid and in the end IsValid is all over the place.

 

The fact that the distance function is called with an invalid entity points to an issue higher up, and patching the low level will only hide the problem, making resulting bugs even harder to find.

tbh it should be something in between 

 

in a normal programming language you would

 

if(invalid)then throw InvalidEntityError(...)

and in code somewhere way above that

catch(InvalidEntityError e){ decide how important is the block of code calling it and whether to try to fix the problem, ignore it, crash}

being that error handling in lua and/or klei's code is non existent, assets are getting thrown to the main thread and crashing the server

the problem is NOT that the check throws an error but that error is not getting caught

i.e. if the code that failed due to invalid entity is definiting an animation effect on the objects its safe to warn: parent object invalid and ignore

if the code is supposed to do something of critical importance to the validity of server ti should crash the server itself

removing error handling is not the solution for bad error handling implementation, but 

 

fixing it properly

and that should be done all over the lua code in a completely different manner but , don't think that's gonna happen.

hacking it out for the sake of not crashing now is not the same as properly fixing it

 

edit: at least that's how I see it

[bizziboi]

@rezecib

 

That side note is of course correct, thanks for noting ) It would never hit the error due to the asserts but it`s not as intended for sure.

[bizziboi]

@kraken121

 

Ummm, that is exactly what I said, it should be fixed higher up, and that I am against hacking it out.

 

As long as it's not fixed, asserting or trying to patch it are both as bad, An assert basically states that the data is in a state the program doesn't expect it to be and it can't guarantee proper operation, continuing to run will lead to possibly worse and more mysterious crashes. 

 

And it just so happens that I have been adding code last Friday to aid me in assisting to find the cause of such issues. Unfortunately not all of them are easily found or easily fixed.