search

ValkyrienSkies / Valkyrien-Skies

Moving structures powered by a custom multi-threaded physics engine; in Minecraft.
https://valkyrienskies.org/
Apache License 2.0
160 stars 40 forks source link

Fix for ConcurrentModificationException on ship disassembly (or any other TE removal mayhaps) #445

Closed klassekatze closed 3 years ago

klassekatze commented 3 years ago

Fix for CME. List(s) within tileEntitiesByExtendedData are retrieved with getTileEntitiesToRender and then added to other lists in render-y thread(s). removeTileEntityFromIndex, OTOH, is called in ship disassembly and such, very not-rendery. 99% of the time, no big deal. 1% of the time, the disassembly might remove TEs from a list whilst that list is being shoved into another in render via addAll. Which... then CMEs.

Naturally, this can only occur in singleplayer and/or clientside. Definitely when in singleplayer.

The repro of the crash I was using is 'have a metric biggaton of tile entities in my ship and assemble/disassemble over and over until you roll snake eyes.'

Regardless, this should fix the bug.

Tri0de commented 3 years ago

I can't replicate this bug.

From what I know the render thread is the same as the game thread, so I don't think a CME could even occur.

Can you provide a log of it?

klassekatze commented 3 years ago

Here. Lot of other garbage, but ultimately, in removeIf in removeTileEntityFromIndex,

debug-1.log

klassekatze commented 3 years ago

I can't say I perfectly understand what i going on, but as far as I do understand, a CME there by definition indicates one thread modified a collection, and a different thread was iterating it at the same time. So it can't be the case that everything being touched is the same thread.

klassekatze commented 3 years ago

Perhaps because the mixin isn't checking remoteness? It's hooking Chunk whether it is client or server or both maybe, so in singleplayer, it executes in the 'server' as well as in the 'client' Chunk instances? and I assume the server is a different thread than the 'client', but I don't know off the top of my head that's the case. It seems like it would be.

Rubydesic commented 3 years ago

I can't say I perfectly understand what i going on, but as far as I do understand, a CME there by definition indicates one thread modified a collection, and a different thread was iterating it at the same time. So it can't be the case that everything being touched is the same thread.

This is not true, you can get a CME just by modifying the collection in the same thread while iterating it.

edit: although I don't think that's relevant here

Tri0de commented 3 years ago

I just deleted the code causing this bug in 12ee7d85dd9005403b0d77e1f74d0ef1e47a4ec7.