search

Slimefun / Slimefun4

Slimefun 4 - A unique Spigot/Paper plugin that looks and feels like a modpack. We've been giving you backpacks, jetpacks, reactors and much more since 2013.
GNU General Public License v3.0
959 stars 548 forks source link

Cargo system voids items transferred at specifc times #1819

Open Liruxo opened 4 years ago

Liruxo commented 4 years ago

Description

The cargo system can transfer items to blocks which technically do not exist anymore More specific in the time window between the block being destroyed on the main thread (dropping the current content of the inventory) and the actual removal from the BlockStorage, the cargo system can transfer items to the inventory which will be voided

Steps to reproduce the Issue

  1. Create a simple cargo setup consisting of one cargo manager, one cargo input node and one cargo output node
  2. Configure the cargo system in such way that it is transferring items into a slimefun managed inventory container, such as the "Electric Furnace"
  3. Destroy the Electric Furnace exactly after the TickerTask has processed Block removals and before it starts processing the tick of the cargo manager
  4. Observe how items are transferred into the destroyed Electric Furnace and are destroyed immediately afterwards as the TickerTask processes the removal of the Electric Furnace

From my testing this is a very rare occasion, and as such a very hard bug to reproduce. However, with the help of a debugger which is capable of pausing the async TickerTask, it is reproducible every time

Expected behavior

The item does not transfer, or drops in the world

Environment

TheBusyBiscuit commented 4 years ago

Also to bring this discussion onto here now, a quick and easy fix for this would be to mark any BlockMenu as "obsolete" upon block breaks and then have a BlockMenu#isObsolete() check in the cargo ticker to prevent such inventories from being manipulated.

Liruxo commented 4 years ago

Current Problems

There is more or less no synchronization in Slimefun whatsoever. Currently the workload of ticking blocks is offloaded in the asynchronous TickerTask which commits many Tasks back onto the main thread as synchronous task. However important tasks such as the EnergyNet are still processed asynchronously, which improves performance and poses synchronization challenges.

Current Solutions

Most of the problems are solved by scheduling BlockStorage removals onto the TickerTask to ensure the TickerTask only works on a (mostly) consistent state of the BlockStorage Additionally most block changes while or after a block removal is in process are sudo made consistent by the constant check for the SlimefunItem-ID, and the following disregard of any information if it is not present

Currently Proposed Solution

This is my suggestions: Make the BlockStorage and/or the accesses to it thread-safe, meaning it would be safe to process block ticks with multiple threads not just one asynchronously running TickerTask I would propose some sort of lock system, to ensure only one thread modifies a block at one time. Alternatively it should be possible largely to realize this with the "compute" functions of the already used ConcurrentHashMap

Cons

Liruxo commented 4 years ago

mark any BlockMenu as "obsolete" upon block breaks and then have a BlockMenu#isObsolete() check in the cargo ticker to prevent such inventories from being manipulated.

While this would work for BlockMenus, as they are due to the non-thread-safe nature of the vanilla inventory only accessed synchronously, a very similar issue can be created using the EnergyNet. Using the same described steps it is easily possible to void energy, which is much harder to fix without proper synchronization. While i currently do not know any significant problem based on the following circumstances, addons can easily misuse the BlockStorage in the described ways, destroying possible value data (In form of Items etc):

TheBusyBiscuit commented 4 years ago

I am definitely all for a proper thread-safe handling of all of this but if I am completely honest I am really not that good when it comes to concurrency, at least in Java. I am much more profound in other implementations. So I am very open to any suggestions regarding this topic.

Although I am not sure how a ClickAction would be able to modify BlockStorage data after the block was destroyed? Because a ClickAction can only trigger when there is a Player to click it and if I remember correctly all Inventory Views are terminated on block destruction.

Liruxo commented 4 years ago

Thats correct, they are closed, but that is controlled by the _integrated_removeBlockInfo, which itself is only called by the async TickerTask while processed Block removals, again leaving the gap between the sync Block break (and call of the SlimefunBlockHandler) and the closing of the inventory

On the implementation, i definitely can contribute such thread-safe handling code, but the bulk of the work is probably in creating the backwards-compatibility and converting all the items/blocks in Slimefun (and Addons) to use the new solution (mainly to profit from the optimizations). But having backwards-compatibility, should mean the conversion is not necessary for a fully working Slimefun Plugin, so the work can be spread out or postponed. But as i currently lack performance analysis of the current implementation, and i am certainly not an expert on performance analysis i definitely would like to hear Walshy's opinion especially if he already worked on or even implemented something for this topic. For example, i would prefer a lock based solution, but that might come at a memory and execution time overhead. Alternatively it would also be possible to implement a two thread solution which would only require minor adjustments to the current solution. However, i and to my understanding TheBusyBiscuit, would prefer a "proper" multi-threading approach, even if it has worse performance on only two threads (Main Bukkit/Spigot thread and one asynchronously running thread). Also, as already mentioned, it should be possible to only use synchronized and prebuilt thread-safe methods to avoid the need for managing all the locks in my current design, which is why i would like to know if anyone knows about any performance or code style advantages/disadvantages in such approach

WalshyDev commented 4 years ago

So, yes I had worked on a system. I have that entire thing working (along with migration). I just needed to create the entire abstract layer so we could add more things down the line (for example MySQL). I was just very lazy to figure out a good way to design this.

This was all completely thread-safe. On chunk load things were loaded in asynchronously and then chunk-unload they were saved. My currently solution isn't perfect and still needs some work (for example, it would load a region of chunks rather than the one specific chunk which could create some un-wanted data being stored in memory. However this is still much less than the current solution).

Modifications of block cache is done sync to avoid any concurrent modification issues.

My current working solution gives a big performance boost (especially since there's no stupid JSON parsing to retrieve block data). It will also reduce the average file by about 90% however it may increase some things by a few bytes. Overall though this is a massive reduction in size, performance boost but breaking change.

I think a breaking change on this is completely fine though especially since all data is migrated anyway (with a backup as to avoid any issues if they wanted to downgrade or an issue occurred).

I haven't read this whole thread so I may not have addressed something, feel free to let me know anything else you wish to be informed on. We can all agree the current system is shivers.

Liruxo commented 4 years ago

Is this Branch publicly available? Because i would like to take a look at it, since i am currently in the process of implementing a similar system for my Sliemfun-Addon. Additionally, if you consider it useful, i would like to offer my help for easing out the last problems, as i will probably need to re-integrate the BlockStorage parts in my Addon with such changes anyways

WalshyDev commented 4 years ago

No I don't have it publically available as it simply just doesn't work right currently.

I could provide you the standalone migration tool that (I think it's slightly behind) I used for testing. If that would be of any help. I'm not too sure what exactly you're looking for at the minute though.