BuildCraft|Core

BuildCraft|Core

7M Downloads

Improve handling of fake players

SpaceToad opened this issue ยท 29 comments

commented

See http://mod-buildcraft.com/forums/index.php?topic=599.0. Anyone cares to give it a shot?

commented

I would like to take a look at it. But i would need more details though. Would that be possible to get?

commented

@OrionDevelopment look at the 3 crash reports in the post spacetoad linked, if you want i can get you 3 or 4 more but they are near identical, only the entity is different but the stacktrace is exactly the same

commented

@AEnterprise I would like the stacktraces (Or are they there and I did not see them?) if that is possible. SO i can retrace some function calls as i have no way of rebuilding the setup that created the bug...

commented

they are there, on the top most post i made, those 3 links

  • crash-2014-05-11_00.49.31-server.txt (12.74 kB - downloaded 5 times.)
  • crash-2014-05-11_00.54.20-server.txt (12.09 kB - downloaded 2 times.)
  • crash-2014-05-11_10.07.54-server.txt (11.94 kB - downloaded 1 times.)
commented

Oke found them... Did not see the attachments Will take a look at it.

commented

Hmm it might be. It is stored in the EntityPlayerMP entity cause that is were the handler itself gets the dispatcher. I will look into that tomorrow.

commented

@OrionDevelopment yup - it's in a different folder.

commented

Haha i fixed it before you wrote that....... (Quick spacetoad). Gradle assigns the source directories wrong. (Resources as source (which should be resource directory) and leaves the api folder alone) Luckily that is easily fixable.

commented

Hmm looking at the code for a fix for this. On sending a packet to all players we should check if they all contain a dispatcher.. But what todo if not (Print a warning message and skip the packet)? Any ideas?

commented

Okey looking through it seems that on the moment BC tries to updates its enities the FML Standard network dispatcher is not initialized for a certain EntityPlayerMP.Which brings me to wonder if BC is actually causing this issue. Can we confirm that this happens with only BC installed? Or if similar crashes are happening with other mods? (So without BC installed?)

commented

@OrionDevelopment is there anyway we can check that before trying to send the message?

commented

One other possibility could be that the fake player we create for BuildCraft causes problems? See https://github.com/BuildCraft/BuildCraft/blob/6.0.x/common/buildcraft/core/proxy/CoreProxy.java#L187-L208.

commented

@SpaceToad that is most likely the cause, if it was a problem with players don't having a dispatcher it would have been reported on forge already and fixed, i guess the dispatcher of BC is missing it's handeler

commented

@SpaceToad @AEnterprise yes that could be he source aswell. I will take a loop at it again this evening.

commented

To carry on with that, let me suggest trying to replace all manual "fake" player creations in BuildCraft (such as CoreProxy.createNewPlayer) by the use of Forge's FakePlayerFactory.get. There's an issue with the "weak reference" thing to be worked out.

Thoughts?

commented

@OrionDevelopment any news on this one?

commented

I'm adding protection for now, but I'm not sure how good that is if it's a systematic failure. Really need some insights here.

commented

Note that I have been completely unable to reproduce so far. If anyone has ideas...

commented

i don't have enough knowledge of that to help with that, and i don't realy have the time to look intro that

commented

Excellent catch @psxlover ! That looks like an extremely likely candidate indeed. Now the first release with that one in should be 1079 so we just need to tell people to upgrade. Other cleanups suggested on this ticket makes sense, but less of a priority.

commented

@psxlover Yes that would solve this issue. Indeed a perfect catch :+1

commented

Yeah that is probably a good idea......
I actually could not reproduce the crash myself... again just happened once while debugging the quarry thing. But I could not catch that properly

commented

Perhaps it has something to do with MinecraftForge/FML@42713c6...503da3a

commented

Alright, let's keep this one for the purpose of the cleanup fake players though. I'm taking care of it.

commented

I can do that as well....
Just replace all the calls to CoreProxy.createNewPlayer with ForgePlayerFactory.get?

commented

Nah, it's a bit more involving. You need to manage weak references, check that there's no code still relying on the client side, check other place where we create manually fake players, and so. I'm tracking all of these down. What will be very useful though is some testing once that's all in.

commented

Alright, I believe I made proper changes, and the code should run fine. I'll try to make a quick release with that one in, but don't hesitate to test. I run most of mine in client / server mode.

commented

Don't forget to change the required forge version in buildcraft core.

commented

Oh my - that should have been updated a while ago!