Mod unsafely accesses client world off-thread
embeddedt opened this issue · 63 comments
Bug description
Sound Physics Remastered accesses the client world from the sound thread. The client world is not really thread-safe and doing this can lead to all sorts of issues. In my case, it seems to cause ghost Create mechanical belts to occasionally render in the world with Embeddium. I am not 100% sure of the exact action that causes that issue, but it seems related to block retrieval off-thread triggering creation of block entities as well as other side effects. A stack trace from my debug mixin is attached below so you can see what I mean (it's using SRG names, sorry about that).
To fix this issue properly I believe the mod will need to be refactored so that all block retrieval is done on the main thread and stored in a snapshot data structure that the sound thread then accesses. Otherwise, this will likely continue to cause all sorts of subtle problems that will be very difficult for players/other mod authors to track down.
This issue was discovered on 1.20.1 but likely affects earlier versions too unless the code changed significantly.
java.lang.Exception: null
at net.minecraft.world.level.chunk.LevelChunk.handler$zbb000$yellForOffThread(LevelChunk.java:1338) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:flywheel.mixins.json:instancemanage.InstanceAddMixin,pl:mixin:APP:rubidium.mixins.json:core.render.world.WorldMixin,pl:mixin:A}
at net.minecraft.world.level.chunk.LevelChunk.m_62934_(LevelChunk.java) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:flywheel.mixins.json:instancemanage.InstanceAddMixin,pl:mixin:APP:rubidium.mixins.json:core.render.world.WorldMixin,pl:mixin:A}
at net.minecraft.world.level.chunk.LevelChunk.m_5685_(LevelChunk.java:315) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:flywheel.mixins.json:instancemanage.InstanceAddMixin,pl:mixin:APP:rubidium.mixins.json:core.render.world.WorldMixin,pl:mixin:A}
at net.minecraft.world.level.Level.m_7702_(Level.java:560) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at com.simibubi.create.foundation.block.IBE.getBlockEntity(IBE.java:72) ~[create-1.20.1-0.5.1.f.jar%23151!/:0.5.1.f] {re:classloading}
at com.simibubi.create.foundation.block.IBE.getBlockEntityOptional(IBE.java:53) ~[create-1.20.1-0.5.1.f.jar%23151!/:0.5.1.f] {re:classloading}
at com.simibubi.create.content.kinetics.belt.BeltBlock.m_5939_(BeltBlock.java:370) ~[create-1.20.1-0.5.1.f.jar%23151!/:0.5.1.f] {re:classloading}
at net.minecraft.world.level.block.state.BlockBehaviour$BlockStateBase.m_60742_(BlockBehaviour.java:684) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B,pl:mixin:APP:modernfix-common.mixins.json:bugfix.chunk_deadlock.BlockStateBaseMixin,pl:mixin:APP:ferritecore.blockstatecache.mixin.json:BlockStateBaseMixin,pl:mixin:APP:modernfix-common.mixins.json:perf.reduce_blockstate_cache_rebuilds.BlockStateBaseMixin,pl:mixin:A}
at net.minecraft.world.level.ClipContext$Block.m_7544_(ClipContext.java:62) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:classloading}
at net.minecraft.world.level.ClipContext.m_45694_(ClipContext.java:40) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:classloading}
at net.minecraft.world.level.BlockGetter.m_151358_(BlockGetter.java:63) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at net.minecraft.world.level.BlockGetter.m_151361_(BlockGetter.java:119) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at net.minecraft.world.level.BlockGetter.m_45547_(BlockGetter.java:58) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.raycast(SoundPhysics.java:618) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.evaluateEnvironment(SoundPhysics.java:287) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.processSound(SoundPhysics.java:180) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.sonicether.soundphysics.SoundPhysics.onPlaySound(SoundPhysics.java:147) ~[soundphysics-forge-1.20.1-1.2.1.jar%23155!/:1.20.1-1.2.1] {re:mixin,re:classloading}
at com.mojang.blaze3d.audio.Channel.handler$zjj000$play(Channel.java:533) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:SourceMixin,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:ChannelAccessor,pl:mixin:A}
at com.mojang.blaze3d.audio.Channel.m_83672_(Channel.java) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:SourceMixin,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:ChannelAccessor,pl:mixin:A}
at net.minecraft.client.sounds.SoundEngine.lambda$play$6(SoundEngine.java:404) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,pl:runtimedistcleaner:A,re:classloading,pl:accesstransformer:B,pl:mixin:APP:assets/sound_physics_remastered/sound_physics_remastered.mixins.json:SoundSystemMixin,pl:mixin:A,pl:runtimedistcleaner:A}
at net.minecraft.client.sounds.ChannelAccess$ChannelHandle.m_120157_(ChannelAccess.java:34) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,re:classloading}
at net.minecraft.util.thread.BlockableEventLoop.m_6367_(BlockableEventLoop.java:156) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:computing_frames,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at net.minecraft.util.thread.BlockableEventLoop.m_7245_(BlockableEventLoop.java:130) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:computing_frames,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at net.minecraft.util.thread.BlockableEventLoop.m_18701_(BlockableEventLoop.java:139) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:mixin,pl:accesstransformer:B,re:computing_frames,pl:accesstransformer:B,re:classloading,pl:accesstransformer:B}
at net.minecraft.client.sounds.SoundEngineExecutor.m_120336_(SoundEngineExecutor.java:42) ~[client-1.20.1-20230612.114412-srg.jar%23156!/:?] {re:classloading}
at java.lang.Thread.run(Thread.java:840) ~[?:?] {re:mixin}
Mods installed
- NeoForge 47.1.79
- Embeddium 0.2.9
- Create 0.5.1f
- FerriteCore 6.0.1
- ModernFix 5.9.3
- Sound Physics Remastered 1.20.1-1.2.1
Version 1.4.0 has now been released as alpha for Minecraft 1.19.2, 1.20.1, 1.20.4 and 1.20.6.
I'll keep this issue open for a while. Please let me know if there are any issues.
Thanks again @augustsaintfreytag for fixing the issue and even improving the mods performance!
Appears to be working on Quilt!
To bypass the Create dependency block, add this to your config/quilt-loader-overrides.json
{
"schema_version": 1,
"overrides": [
{
"id": "create",
"breaks": [
{
"replace": {
"id": "sound_physics_remastered",
"versions": "*"
},
"with": {
"id": "sound_physics_remastered",
"versions": "<1.4.0"
}
}
]
}
]
}p.s. I have been putting together a modpack over the last few days and I was JUST about to say bye to Sound Physics, but I'm glad I saw this! Thanks for all the hard work!
Only just realised that you're one of the maintainers of Create, @IThundxr, nice to see you here, too. I didn't know about the fabric_loader_dependencies.json, I just hacked it in to test it, this should be the way for anyone wanting to test the preview build, though, I agree.
@RyanTheTechMan, you could replace the * with >=1.4.0, that's where we'll be at with this patch, only the preview build could satisfy this at the moment so no one can accidentally load an outdated version.
If @CelestialAbyss still follows this thread, I'd be very interested if they still see these artifacts they showed above, just to have another confirmation. Technically there should be no way they still appear.
@RyanTheTechMan, you could replace the
*with>=1.4.0, that's where we'll be at with this patch, only the preview build could satisfy this at the moment so no one can accidentally load an outdated version.
Just edited the message. My b
Edited to <1.4.0 as it breaks anything below 1.4.0, but not 1.4.0 itself.
@ImBehinYa Compatibility is defined within Create not here and they've flagged all versions of Sound Physics as incompatible. For testing, I just went into the fabric.mod.json in Create's JAR file and edited the version check for Sound Physics, you could give that a try if you want to demo my fork, mine is "2.0.0".
@AtlasM8 I got side-tracked a bit with other mod projects but I've put it all together now, I'm gonna open the merge request this weekend, just need to summarise my changes now, shouldn't be long!
Thank you @augustsaintfreytag for your PR!
I compiled both the Fabric and Forge version for people to test:
sound-physics-remastered-1.20.1-1.4.0-pre1.zip (GitHub doesn't like Jars, so I had to zip them)
I'd love feedback from everyone that had concurrency issues.
@ImBehinYa Compatibility is defined within Create not here and they've flagged all versions of Sound Physics as incompatible. For testing, I just went into the
fabric.mod.jsonin Create's JAR file and edited the version check for Sound Physics, you could give that a try if you want to demo my fork, mine is "2.0.0".@AtlasM8 I got side-tracked a bit with other mod projects but I've put it all together now, I'm gonna open the merge request this weekend, just need to summarise my changes now, shouldn't be long!
use dependency overrides over modifying jars https://fabricmc.net/wiki/tutorial:dependency_overrides
"Hi, I just want to understand if it should work with create when I compile the fork. And when can we expect compatibility again?"
I can confirm that 1.4.0 fixes the symptoms reported in my initial post - there are no longer CMEs and other errors printed in the log when breaking belts, and belts no longer continue rendering after being broken.
Why not just wrapping the level to avoid unsafe calls such as getBlockEntity? That seems to be what's causing the issue above
The suggestion above looks like it would fix it to me. But only way to know for sure is for it to be implemented and tested.
Marked SPR as broken in create fabric due to the mass amounts of concurrency issues we've gotten, If the suggestion above works and fixes this please let me know so i can remove the breaks.
Sound Physics is an amazing mod and is definitely worth it to see this change implemented. I'd say it gives it some solid future-proofing to use an access-by-snapshot approach, especially if vanilla already does it this way.
The create latest patch actually added an incompatibility flag for this mod, so now you can't play with both installed because of this non thread safety.
The create latest patch actually added an incompatibility flag for this mod, so now you can't play with both installed because of this non thread safety.
Yes, as mentioned here:
just to be clear. Is this planned to be worked on?
Looks like it was reopened, pinned, and marked as a bug. If not immediately it looks like there will at least be an attempt to fix it.
Looks like it was reopened, pinned, and marked as a bug. If not immediately it looks like there will at least be an attempt to fix it.
Yes, I currently don't have the capacity to look into it, but its on my list. If anyone is interested to take a look at it or make a PR, any help is always appreciated!
@AtlasM8 I don't see a reason it couldn't be ported to all versions Sound Physics currently supports (which seem to be 1.19.2, 1.20.1, and 1.20.4). From my changes, it only depends on how much the internals of levels have changed between releases. I haven't built on the most recent Minecraft version myself, I branched off 1.20.1 because it's the mod LTS right now (and it's the one I play on).
Ok. I decided to switch my mod pack to 1.20.1 anyway since writing that post. Keep up the good work!
So I decided I didn't want to live without Sound Physics (mainly because of the Voice Chat integration) so I took a look at a possible implementation myself. I took the direction @embeddedt proposed here and wrote a working thread-safe proof of concept.
I checked what data SPR actually uses and it's pretty straightforward, block state and clip for everything from materials to raycasting (reflection and occlusion). The team behind Sodium/Embeddium also creates snapshots in a LevelSlice model, though obviously, you need much more of the level data for rendering than for sound simulation so I was able get by with cloning less than they do.
My approach lets SPR work entirely off a sparse level snapshot inside a ClientLevelProxy that offers the functionality otherwise provided by ClientLevel. Currently, I create it for every run of evaluateEnvironment, though that can be optimised. It takes the player's position and copies block data in a configurable radius into a collection of ClonedLevelChunk models. This copy is completely decoupled from ClientLevel but provides everything we need, block states, fluid states, height maps. SoundPhysics itself then creates a proxy once and uses that instead of the full level with the exact same signatures as before.
As I see it, this is the breakdown for what I created:
🟡 Disadvantages
- Introduce a brief halt having to synchronize the client level's
ClientChunkCacheto clone it. During initial testing, the performance impact for this is neglible, even with a naive and overeager approach (copy for every sound). There is immense potential for optimisation, e.g. by keeping level data around for more than one sound event, reducing caching radius, etc. - Slightly increase memory use and bandwidth to accommodate the cloned chunk data, though this can also be optimised by reusing copied data or reducing the copied chunk radius.
🟢 Advantages
- Make sound simulation thread-safe in regards to level data and whatever happens on the main thread while keeping everything on the sound thread.
- Fix threading-related errors where accessed level data is invalid, solve incompatibility with Create and other mods yet unreported.
- (New) Potentially increase overall sound simulation performance by allowing the mod to only use part of the available level data for evaluation (i.e. use an 8x8 chunk radius around the player instead of the whole client-loaded level for operations like raycasting). The radius to be cached can be configured.
I'm going to refine and clean up my branch a bit but I wanted to announce this here now that I can confirm it's working. I also tested it in a big pack of 400+ mods (including Create and addons) and haven't encountered any issues there.
So I decided I didn't want to live without Sound Physics (mainly because of the Voice Chat integration) so I took a look at a possible implementation myself. I took the direction @embeddedt proposed here and wrote a working thread-safe proof of concept.
I'm following this thread. Post a link to the mod in here once you upload it. I will definitely be trying out your version when it drops.
To anyone else about to comment to receive notifications on this issue, you can subscribe to issues! Just click the subscribe button!
I'm going to open a merge request soon if @henkelmax would want this to be merged into their fork after review, there are just a few small things I want to add and I haven't had a chance to profile it on the actual numbers, then I'll get to it.
I think it'd be in the community's best interest if I don't create an entirely separate fork of Sound Physics with these changes and instead, we use what I did to reinstate this project and Max's work on the mod.
If anyone is overeager to test it, I do have my fork as a public repo, all my WIP changes are on feature/thread-safety, you can compile it and test it on 1.20.1. All the debug tools already included in Sound Physics work just as fine.
@augustsaintfreytag My only feedback on your current implementation is that you currently create the level snapshot in SoundPhysics.evaluateEnvironment, which is already running on the sound thread. For proper thread safety the snapshot needs to be created on the client thread instead.
Since from what I can tell, you just copy a range of chunks around the player, the simplest solution might be to maintain the snapshot in a client tick event, and update it every 20 ticks, or when the player has moved a given distance. The sound thread can then just use the snapshot being maintained by the client thread.
@embeddedt I thought about that approach, too, to always have something like an atomic level clone ready on the main thread that'd be safe to access from the sound thread. My current draft synchronizes access to the ClientChunkCache during the clone in ClonedClientLevel so the data is locked during the read operation. I can get on board with moving from this "cache-on-sound" to a "cache-always" approach, sounds probably happen frequently enough to warrant it.
All in all, I think this discussion might be best held in the merge request later. Like I said, this was my proof of concept implementation, I'm happy to refine it with you all. I also have a few mod-specific things to ask Max on what we might want to make configurable, e.g. caching/eval radius or cache retain time.
I appreciate you checking it out already, your Discord was the one I was thinking to ask around before I started on the implementation.
What about Create compatibility? Will it be fixed or no
that's fixed in dev already
What about Create compatibility? Will it be fixed or no
that's fixed in dev already
Oh yea just noticed, sorry. Seems like we need to create a config to fix it, thats probably now in Create's side since they marked the mod as broken.
I just released 1.4.1. This version fixes some potential issues with changing levels or leaving/joining the game.
Would have left this comment on a different issue but I was 10 minutes late and it got locked lol.
The next version of Create Fabric is expected to remove that declaration of incompatibility. Fabricators-of-Create/Create@b12d4d8
Until then, you can create a Fabric dependency override:
Create a file in your config folder with the name fabric_loader_dependencies.json and the following contents:
{
"version": 1,
"overrides": {
"create": {
"breaks": {
"sound_physics_remastered": "<1.20.1-1.4.0"
}
}
}
}
Downloadable file for your convenience: fabric_loader_dependencies.json
In case it still happens, I'd appreciate if you'd send me logs of that version again.
Would have left this comment on a different issue but I was 10 minutes late and it got locked lol.
The next version of Create Fabric is expected to remove that declaration of incompatibility. Fabricators-of-Create/Create@b12d4d8
Until then, you can create a Fabric dependency override:
Create a file in your
configfolder with the namefabric_loader_dependencies.jsonand the following contents:{ "version": 1, "overrides": { "create": { "breaks": { "sound_physics_remastered": "<1.4.0" } } } }
Your override is not correct. <1.4.0 would also allow 1.20.1-1.3.0, as your check only checks the first part of the mod version which is the Minecraft version.
One user of my modpack reported deadlock.
Idk deadlock related or not with SPR but can see 10k lines of spam from SPR mod before deadlock.
[02:30:11] [Sound engine/WARN]: Can not return client level proxy, client level clone has not been cached. This might only occur once on load.
Please provide the full log files of this crash.
Please also make sure this also happens in the latest version of the mod.
This isn't a specific crash report. It's a technical explanation of a design flaw that I believe is responsible for causing random glitches or crashes in other mods, plus an example stacktrace outlining how the problem occurs.
Here's a screenshot of one of the issues encountered on 1.19.2 after installing SPR:
The mod did always access the client world from another thread. This was already a thing back then for the original mod from Sonic Ether. The mod is designed around being able to do this. I also think this mod wouldn't be feasible if blocks need to be accessed from the main thread. If all evaluation rays need to be casted in the main thread, the game frame rate would be unplayable. For now this has always worked without any issues. Its definitely possible that this causes the issues with the create mod, but usually issues from non-thread safe operations are way more inconsistent, so I'm assuming create does some very weird hacky stuff to the world rendering or changes very fundamental stuff. I also always used the mod in conjunction with Sodium and never had any issues. Maybe Embeddium does some things differently.
Interactions from off thread are also limited to read-only calls, which in general shouldn't cause many issues.
All I can do is declare Create or Embeddium as an incompatible mod, as its impossible to make the mod "Thread safe". If anyone has any ideas on how to work around this issue, please let me know, but Thread safety isn't really possible to my knowledge.
If anyone has any ideas on how to work around this issue, please let me know, but Thread safety isn't really possible to my knowledge.
The only way to do it correctly (as far as I know) is to make a snapshot of the needed block data on the main thread, and then work with that on the sound thread (without ever accessing the client world directly). This is the same approach vanilla uses to update the chunk meshes using worker threads (16x16x16 snapshots are made on main and then used by the workers).
Anyways, the next release of Embeddium will warn users that support won't be provided if Sound Physics (or other unsafe threading mods like Entity Culling) are installed, as I'm getting too many concurrency-related bug reports which are not caused by me, and that I don't have an easy way to fix.
This also causes https://mclo.gs/cWFrIAu by creating a CME when light updater is invoked since it is not thread safe
This is out for a while now and it seems that there are no major issues, so I'll close this. Thanks again to everyone that helped with this issue!
For anyone who want to do a quick fix: Fabricators-of-Create/Create#1503 (comment)
This should already be fixed with 1.4.2
@henkelmax
Hey, oddly the issue might be back? Not sure, but the logs do seem similiar. The game freezes on loading into the server, but after (some, sometimes about 3minutes, it loads fine) https://mclo.gs/XXNEB5w
[14:44:05] [Sound engine/WARN]: [LevelAccessUtils#getClientLevelProxy] Can not return client level proxy, client level clone has not been cached. This might only occur once on load.
@augustsaintfreytag there might be an issue with the caching in combination with a certain mod.
@tajemniktv Could you check which mod in combination with SPR causes the issue?
So I've got the newest version of Create Fabric and Sound Physics Remastered and I'm getting a error that doesn't let the game start at all. I've tried troubleshooting and looking through this thread and one that was marked as spam and directed to this one, but I haven't found anything. I dunno what to do or if there is a version of either mod that will allow me to use the other.
Correct, use a dependency override like people have mentioned above or wait for the next patch
I’m not sure. Check the messages above and see if one of the possible solutions apply to your case
forge create0.5.1h also has this bug
1.19.2-latest
It is known, create only has to update now for it to work. There’s a temporal fix as stated above by making an override file
forge create0.5.1h也有这个bug
1.19.2-最新
众所周知，create 现在只需更新即可工作。如上所述，可以通过创建覆盖文件来临时修复
how？
Only just noticed the discussion here, is there a confirmed deadlock issue in SPR? I'd say it could only happen in the caching logic, the only part that happens on the main thread. And it's not like building an initial cache would take longer than any future pass. I personally haven't encountered anything suspicious yet and I do play with quite a number of mods. If people are having issues (@KostromDan), I could take a look, though, if this happens in SPR > 1.4.0.
@tajemniktv If this is actually a server log, none of the sound physics code should actually be running server-side, so that'd be the heart of the issue. That line is logged if a sound is played before the main thread mixin had a chance to cache a radius of the client world which it should always do the first tick. If this runs server-side, there never would be a cache to access so I can picture this just being logged eternally, for every played sound. Not sure why it would cause a lock-up, though. I see this is 1.21.x, there may have been changes I don't know about, haven't touched that version.
On the Create topic, at least on 1.20.1 releases for Fabric, the compatibility rule has been updated for months already, we've had maintainers of Create in this thread following along. I can only imagine this being a problem for older versions or forks of Create.