[Java] `ThreadSafeFory` does not appear to be thread-safe when registering classes with `createSerializer` = true

[drse]

Search before asking

• I had searched in the issues and found no similar issues.

Version

0.11.0

Component(s)

Java

Minimal reproduce step

(0..1000).collect {
    new Thread(() -> {
        Fory.builder()
            .withLanguage(Language.JAVA)
            .requireClassRegistration(true)
            .withAsyncCompilation(true)
            .withCompatibleMode(CompatibleMode.COMPATIBLE)
            .buildThreadSafeForyPool(2, 4, 1, TimeUnit.MINUTES)
            .register(MyClass.class, true) // register custom class and create serializer
        }
    )
}.collect {
    it.start()
    it
}.each {
    it.join()
}

What did you expect to see?

No errors

What did you see instead?

java.lang.RuntimeException: Create sequential serializer failed, 
class: class MyClass
	at org.apache.fory.serializer.CodegenSerializer.loadCodegenSerializer(CodegenSerializer.java:52)
	at org.apache.fory.resolver.ClassResolver.lambda$getObjectSerializerClass$4(ClassResolver.java:1138)
	at org.apache.fory.builder.JITContext.lambda$registerSerializerJITCallback$0(JITContext.java:91)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1095)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:619)
	at java.base/java.lang.Thread.run(Thread.java:1447)
Caused by: java.util.ConcurrentModificationException
	at java.base/java.util.HashMap.computeIfAbsent(HashMap.java:1230)
	at org.apache.fory.resolver.ClassResolver.getClassDef(ClassResolver.java:1747)
	at org.apache.fory.builder.ObjectCodecBuilder.lambda$new$0(ObjectCodecBuilder.java:97)
	at org.apache.fory.builder.JITContext.asyncVisitFory(JITContext.java:159)
	at org.apache.fory.builder.BaseObjectCodecBuilder.fory(BaseObjectCodecBuilder.java:225)
	at org.apache.fory.builder.ObjectCodecBuilder.<init>(ObjectCodecBuilder.java:94)
	at org.apache.fory.builder.CodecUtils.loadOrGenObjectCodecClass(CodecUtils.java:43)
	at org.apache.fory.serializer.CodegenSerializer.loadCodegenSerializer(CodegenSerializer.java:49)
	... 5 more

Anything Else?

The culprit seems to be line 242 in ClassResolver.java:

private final Map<Class<?>, ClassDef> classDefMap = new HashMap<>();

Changing that to ConcurrentHashMap would probably solve the issue, but I'm not sure if that would break any other contracts.

Are you willing to submit a PR?

• I'm willing to submit a PR!

[moooonk]

The concurrency exception may be caused by incorrect usage. I believe buildThreadSafeFuryPool should not be called within each thread, but rather should be constructed externally once and then shared by all threads.

[drse]

This is true, it's mostly showing up in unit tests which are ran concurrently, not in my main application where a singleton instance of fory is created and shared. However, it's causing the specs to fail, and it would be incorrect to use a shared instance across unit tests, though of course possible.

[moooonk]

Whether to modify the code to prevent incorrect usage patterns from throwing exceptions, or retain this error behavior to alert users about misuse, is considering.

[drse]

True. I think allowing incorrect usage would cause all sort of resource issues that would go undetected. However, if it is intended that only a single pool is created, should the builder not return a singleton instance of fury pool and disallow creation of multiple pools?

[chaokunyang]

We use lock in JITContext for thread safety, all code executed in ClassResolver should be protected by this lock. It's strange why ClassResolver.getClassDef not be protected

[chaokunyang]

ClassResolver is designed to be used only in signle thread, we won't use things like ConcurrentHashMap in ClassResolver

[chaokunyang]

The concurrency exception may be caused by incorrect usage. I believe buildThreadSafeFuryPool should not be called within each thread, but rather should be constructed externally once and then shared by all threads.

You are right, but even called multiple times in different threads, this exception should not occur still. It's pretty strange, the lock in JITContext is a pretty big lock

[moooonk]

@chaokunyang Understood, I'll take a look why concurrent exceptions are still being thrown despite using big lock.

[chaokunyang]

Fixed in #2365, the registration may generate serializer, which is not protected