karashiiro / MachinaServer

A simple Machina wrapper to send segments to an HTTP server for easy IPC.
GNU General Public License v3.0
13 stars 9 forks source link

Exception thrown in MachinaWrapper.Parsing.Parser.Parse when zero size packet is received #41

Closed apmorton closed 4 years ago

apmorton commented 4 years ago

This is more likely a bug upstream somewhere, as I don't think MessageReceived should get called with a zero size data array, but its probably best to gracefully handle the situation.

Currently this exception bubbles up here, and causes the machina TCPNetworkMonitor thread to abort silently.

Index was out of range. Must be non-negative and less than the size of the collection.
Parameter name: startIndex
   at System.ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument argument, ExceptionResource resource)
   at System.BitConverter.ToUInt32(Byte[] value, Int32 startIndex)
   at MachinaWrapper.Parsing.Parser.Parse(Packet meta) in C:\Users\amorton\source\repos\ffxiv\MachinaWrapperJSON\MachinaWrapper\src\Parsing\Parser.cs:line 75
   at MachinaWrapper.MachinaWrapper.MessageReceived(String connection, Int64 epoch, Byte[] data) in C:\Users\amorton\source\repos\ffxiv\MachinaWrapperJSON\MachinaWrapper\MachinaWrapper.cs:line 165
   at MachinaWrapper.FFXIVNetworkMonitor.OnMessageReceived(String connection, Int64 epoch, Byte[] message) in C:\Users\amorton\source\repos\ffxiv\MachinaWrapperJSON\MachinaWrapper\FFXIVNetworkMonitor.cs:line 55
   at MachinaWrapper.FFXIVNetworkMonitor.ProcessReceivedMessage(String connection, Byte[] data) in C:\Users\amorton\source\repos\ffxiv\MachinaWrapperJSON\MachinaWrapper\FFXIVNetworkMonitor.cs:line 137
   at MachinaWrapper.FFXIVNetworkMonitor.<Start>b__26_1(String connection, Byte[] data) in C:\Users\amorton\source\repos\ffxiv\MachinaWrapperJSON\MachinaWrapper\FFXIVNetworkMonitor.cs:line 96
   at Machina.TCPNetworkMonitor.OnDataReceived(String connection, Byte[] data) in C:\Users\amorton\source\repos\ffxiv\machina\Machina\TCPNetworkMonitor.cs:line 82
   at Machina.TCPNetworkMonitor.ProcessData(Byte[] buffer, Int32 size) in C:\Users\amorton\source\repos\ffxiv\machina\Machina\TCPNetworkMonitor.cs:line 340
   at Machina.TCPNetworkMonitor.ProcessNetworkData() in C:\Users\amorton\source\repos\ffxiv\machina\Machina\TCPNetworkMonitor.cs:line 310
   at Machina.TCPNetworkMonitor.Run(Object state) in C:\Users\amorton\source\repos\ffxiv\machina\Machina\TCPNetworkMonitor.cs:line 209

It is choking on this line (where Offsets.PacketSize = 0):

meta.PacketSize = BitConverter.ToUInt32(meta.Data, (int)Offsets.PacketSize);
karashiiro commented 4 years ago

I've never heard of this happening before, but I just pushed an early return for the case, hope that solves it.

apmorton commented 4 years ago

Thanks, I think that should take care of it.

Most users would likely not notice this issue beyond teamcraft randomly ceasing to update inventory data and the MachinaWrapper process starting to consume more and more memory until teamcraft is restarted. No error messages appear and nothing actually crashes, the thread that processes network packets in machina silently exits - but the incoming packet queues are still filled, causing them to balloon over time.

I had to specifically go looking for this issue in a debugger to catch what was actually going on.

karashiiro commented 4 years ago

Oh, that's what was causing that? Thanks for looking into it 😄 A number of other people tried profiling the application as well, but none of them were able to figure that out.

apmorton commented 4 years ago

For the record, there is also another bug I discovered first in machina that results in similar behavior.

https://github.com/ravahn/machina/issues/13

The bug in machina results in ballooning memory consumption and excessive CPU usage (100% of a single core).

This bug will present as ballooning memory only.

In general any unhandled exceptions in the event handlers you register on the machina TCPNetworkMonitor will cause it's internal thread to silently terminate and trigger this bug. It may be a good idea to catch and log all exceptions in the following functions:

https://github.com/karashiiro/MachinaWrapperJSON/blob/master/MachinaWrapper/FFXIVNetworkMonitor.cs#L115 https://github.com/karashiiro/MachinaWrapperJSON/blob/master/MachinaWrapper/FFXIVNetworkMonitor.cs#L128

Alternatively, we can try and get some help from @ravahn to handle exceptions on the machina side gracefully.

karashiiro commented 4 years ago

I'll add try/catches around both of those methods and make sure they hit the log so Teamcraft's testers will be able to see them if they appear as well, thanks for the heads-up.

As for Ravahn, he's usually pretty unresponsive on Github, you'll have a much better chance contacting him on Discord @Ravahn#0248.

Caittus commented 4 years ago

I had the ballooning memory/failure to update inventory bug again, but this time it threw an error message as well after a bit, don't know if it'll help or not but it was suggested I bring it to github. Machina

karashiiro commented 4 years ago

Hopefully "fixed" with https://github.com/karashiiro/node-machina-ffxiv/commit/6d82799ac4e1a08b9ba0f2c23a8e2b3f07adee4a, memory ballooning will still happen until ravahn fixes his stuff, but it won't crash like this anymore, hopefully.

apmorton commented 4 years ago

Yeah, the root cause here is the memory ballooning causing your OS to kill the machinawrapper process. Ravahn has a beta that in theory should fix the issue based on my feedback, but he hasn't published the corresponding machina source anywhere yet, so we just have to wait.