Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Solve class definition race conditions #1949

Merged
merged 1 commit into from
Aug 12, 2023

Conversation

Moulberry
Copy link
Contributor

My mod uses the vanilla ModelBlockRenderer to tesselate blocks on the main render thread. Unfortunately, there exists 2 race conditions in VertexSerializerFactory which results in LinkageErrors being thrown due to duplicate class definition. See below for example stacktrace.

This PR does 2 things:

  1. Calls createSerializer after acquiring the write lock
  2. After acquiring the write lock, does an additional check to see if cache was already written to. As explained, this secondary check is necessary due to 'find' using a non-exclusive lock

Example crash without this fix:

java.lang.LinkageError: loader net.fabricmc.loader.impl.launch.knot.KnotClassLoader @731a74c attempted duplicate class definition for me.jellysquid.mods.sodium.client.render.vertex.serializers.generated.VertexSerializer$Impl$0005$0008. (me.jellysquid.mods.sodium.client.render.vertex.serializers.generated.VertexSerializer$Impl$0005$0008 is in unnamed module of loader net.fabricmc.loader.impl.launch.knot.KnotClassLoader @731a74c, parent loader net.fabricmc.loader.impl.launch.knot.KnotClassLoader$DynamicURLClassLoader @4b4523f8)
	at java.base/java.lang.ClassLoader.defineClass0(Native Method)
	at java.base/java.lang.System$2.defineClass(System.java:2307)
	at java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClass(MethodHandles.java:2439)
	at java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClass(MethodHandles.java:2416)
	at java.base/java.lang.invoke.MethodHandles$Lookup.defineClass(MethodHandles.java:1843)
	at me.jellysquid.mods.sodium.client.render.vertex.serializers.generated.VertexSerializerFactory.define(VertexSerializerFactory.java:225)
	at me.jellysquid.mods.sodium.client.render.vertex.serializers.VertexSerializerRegistryImpl.createSerializer(VertexSerializerRegistryImpl.java:81)
	at me.jellysquid.mods.sodium.client.render.vertex.serializers.VertexSerializerRegistryImpl.create(VertexSerializerRegistryImpl.java:50)
	at me.jellysquid.mods.sodium.client.render.vertex.serializers.VertexSerializerRegistryImpl.get(VertexSerializerRegistryImpl.java:43)
	at com.mojang.blaze3d.vertex.BufferBuilder.copySlow(BufferBuilder.java:582)
	at com.mojang.blaze3d.vertex.BufferBuilder.push(BufferBuilder.java:572)
	at me.jellysquid.mods.sodium.client.render.immediate.model.BakedModelEncoder.writeQuadVertices(BakedModelEncoder.java:95)
	at com.mojang.blaze3d.vertex.BufferBuilder.putBulkData(BufferBuilder.java:1563)
	at net.minecraft.client.renderer.block.ModelBlockRenderer.putQuadData(ModelBlockRenderer.java:144)
	at net.minecraft.client.renderer.block.ModelBlockRenderer.renderModelFaceAO(ModelBlockRenderer.java:120)
	at net.minecraft.client.renderer.block.ModelBlockRenderer.tesselateWithAO(ModelBlockRenderer.java:77)

@jellysquid3
Copy link
Member

Ouch! That's a terrible bug on our part.

Maybe we should use a ConcurrentHashMap instead? I believe if two threads call ConcurrentHashMap.computeIfAbsent then only one thread will compute the value, and both threads will be returned that value. That would also ensure the registry doesn't stay locked for too long while defining a class.

@jellysquid3 jellysquid3 added this to the Sodium 0.5.1 milestone Aug 9, 2023
@jellysquid3 jellysquid3 merged commit a2d71b8 into CaffeineMC:dev Aug 12, 2023
1 check passed
IMS212 pushed a commit to IMS212/sodium-fabric that referenced this pull request Aug 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants