Peripherals should attach/detach on the main thread
SquidDev opened this issue ยท 4 comments
Minecraft Version
1.19.x
Version
1.103.1
Details
Currently IPeripheral.attach
/IPeripheral.detach
may be called on either the computer thread (if the computer turns on/turns off) or the main thread (if attached while the computer is running).
When attach
/detach
runs on the computer thread, this means you have to be careful about interacting with the Minecraft world. This is often hard to get right - for instance, when attaching a drive with a fresh disk in it, it would mutate the underlying ItemStack
, which theoretically could cause problems.
In edb21f3 we attempted to fix this by deferring creating the (Writable)Mount
until the next tick. However, this means that the disk may not mounted by the time the computer turns on, meaning disk startup does not work (SwitchCraftCC/issues#31).
I think ideally we'd change IPeripheral.attach
/IPeripheral.detach
to always run from the main thread. I don't know how this should be implemented in practice yet - currently attaching is done during PeripheralAPI
's startup phase: we could split computer startup into several phases (this may be a good idea anyway), with an intermediary one being run on the main thread, but not sure if it's the best option.
don't know how this should be implemented in practice yet
So attaching is straightforward (as mentioned above, we can do this when ticking the computer), though I would hesitate to call it easy.
However, I'm not sure how to handle detaching yet. Once a computer is shutdown (or at least removed from the ServerComputerRegistry
), it will no longer be ticked, so we can't easily defer calling .detach()
.
Maybe we need to make the computer registry hold onto computers for longer - we possibly need this for persistence too, not sure!
I was hoping to do this after the computer thread rework in e3ced84, but looking at this again, I'm less sure this idea is sensible in the first place.
When we attach a disk drive we need to construct a mount, which in turn requires scanning the whole FS to compute the remaining space. This should be pretty quick (disks are limited to 125KB), but probably still something we want to avoid doing on the main thread too often.
There's possibly an argument here that peripherals should always be attached on the computer thread. This used to be the case until 474f571, but maybe it's worth revisiting. This does have some problems though - a peripheral may be added to the map but not actually attached - so not clear if this is the best solution either.
I think the safest option for now is to leave things as-is, and fix disk drives separately. If we change the IMedia
API slightly, I think we can have something which is thread safe and preserves existing behaviour.
I have discovered a truly marvellous alternative solution to #1649, which this comment field is too narrow to contain.