[Urgent] Concurrency issue when a @Async method is first time accessed

[knuclechan]

I encounter an ConcurrentModificationException as shown below.

It happens when a @ Async method is first time called by more than 1 threads. Say, if there's 2 thread concurrently access the method, one of them goes through, another thread will encounter ConcurrentModificationException.

03-25 14:43:14 ERROR - 
java.lang.RuntimeException: java.util.ConcurrentModificationException
	at io.github.vipcxj.jasync.runtime.java8.helpers.IndyHelpers.createFunction(IndyHelpers.java:83) ~[jasync-runtime-0.1.9.jar:?]
	Suppressed: reactor.core.publisher.FluxOnAssembly$OnAssemblyException: 
Assembly trace from producer [reactor.core.publisher.MonoError] :
	reactor.core.publisher.Mono.error(Mono.java:314)
	[SKIPPED...]
Original Stack Trace:
		at io.github.vipcxj.jasync.runtime.java8.helpers.IndyHelpers.createFunction(IndyHelpers.java:83) ~[jasync-runtime-0.1.9.jar:?]
		at io.github.vipcxj.jasync.runtime.java8.helpers.IndyHelper.voidPromiseFunction(IndyHelper.java:139) ~[jasync-runtime-0.1.9.jar:?]
		[SKIPPED...]
		at java.lang.Thread.run(Thread.java:833) [?:?]
Caused by: java.util.ConcurrentModificationException
	at java.util.HashMap.computeIfAbsent(HashMap.java:1221) ~[?:?]
	at io.github.vipcxj.jasync.runtime.java8.helpers.IndyHelpers.createFunction(IndyHelpers.java:51) ~[jasync-runtime-0.1.9.jar:?]
	at io.github.vipcxj.jasync.runtime.java8.helpers.IndyHelper.voidPromiseFunction(IndyHelper.java:139) ~[jasync-runtime-0.1.9.jar:?]
	[SKIPPED...]
	at java.lang.Thread.run(Thread.java:833) [?:?]

I look into it a bit, and I am pretty sure a lock is required at here.

IndyHelpers.java (Original)

 protected  <T> T createFunction(
            Class<T> proxyType, String proxyMethodName, MethodType proxyMethodType,
            String implMethodName, MethodType implMethodType,
            boolean isStatic, Object thisObj, Object... args
    ) {
        try {
            final CallSite site = callSites.computeIfAbsent(implMethodName, k -> {

Suggested change:

 protected  <T> T createFunction(
            Class<T> proxyType, String proxyMethodName, MethodType proxyMethodType,
            String implMethodName, MethodType implMethodType,
            boolean isStatic, Object thisObj, Object... args
    ) {
        try {
            // Get it without locking first
            CallSite site = callSites.get(implMethodName);
            if(site == null) {
                synchronized {
                    //computeIfAbsent is still required here, as there may be multiple threads pass the if condition
                    site = callSites.computeIfAbsent(implMethodName, k -> {

This issue is super important to fix. Would you please release a new version after fixing it? Thanks a lot!

[vipcxj]

Can you supply a unit test case? I need a unit test to make sure the issue is actually fixed and to avoid it being triggered again in the future

[knuclechan]

Here you go

import static org.junit.jupiter.api.Assertions.assertTrue;

import java.time.Duration;

import org.junit.jupiter.api.Test;

import io.github.vipcxj.jasync.reactive.Promises;
import io.github.vipcxj.jasync.spec.JAsync;
import io.github.vipcxj.jasync.spec.JPromise;
import io.github.vipcxj.jasync.spec.annotations.Async;
import reactor.core.publisher.Mono;

public class ConcurrentFirstTimeAccessTest {
	
	@Test
	public void testAsyncConcurrentFirstTimeAccess() {
		Thread t1 = new Thread(this::job1);
		Thread t2 = new Thread(this::job2);
		
		t1.start();
		t2.start();
		
		try {
			t1.join();
			t2.join();
		} catch (Exception e) {}
	}
	
	private void job1() {
		System.out.println("[1] Thread start");
		boolean success = false;
		try {
			Mono<Boolean> result = doSomething().unwrap(Mono.class);
			result.block();
			System.out.println("[1] Success");
			success = true;
		} catch (Exception e) {
			System.out.println("[1] Error");
			e.printStackTrace();
		}
		
		assertTrue(success);
	}
	
	private void job2() {
		System.out.println("[2] Thread start");
		boolean success = false;
		try {
			Mono<Boolean> result = doSomething().unwrap(Mono.class);
			result.block();
			System.out.println("[2] Success");
			success = true;
		} catch (Exception e) {
			System.out.println("[2] Error");
			e.printStackTrace();
		}
		
		assertTrue(success);
	}
	
	@Async
	private JPromise<Boolean> doSomething(){
		Promises.from(Mono.delay(Duration.ofMillis(100L))).await();
		
		return JAsync.just(Boolean.TRUE);
	}
	
}

[vipcxj]

I will release new version to fix it today.
I think changing callSites from HashMap to ConcurrentHashMap fix this issue, too.

[knuclechan]

Thanks. ConcurrentHashMap works as well.

[vipcxj]

Have released. It might take a while for you to find it in maven centeral repo.

[knuclechan]

Sorry, I find that the version 0.1.10 does not include the fix and the problem is still there. Would you please help to check?

[vipcxj]

@knuclechan
It's version 0.1.11, not 0.1.10. And The next generation JAsync has released (1.X.X), more powerful and not rely on other impl such as react. but it's not statable now. I will update the readme when it is statable.

[knuclechan]

Sorry, my bad.
Looking forward for the stable next generation version.

btw, I noticed some not very critical issue on the old version. But I haven't got the time to repeat it or to write a test case for you. Just hope you can be aware of it

1. Cannot call a method in argument

This will fail to compile (not always fail, I cannot find the exact condition)

Promises.from(userService.findUserById(user.getUserId()))

But this is ok

Integer userId = user.getUserId();
Promises.from(userService.findUserById(userId));

2. If there are more than 4 layer of if clause, it fails (I forget if it is runtime or compile time failure). But I can avoid this by moving the if clauses in a new method.

[vipcxj]

@knuclechan
Currently my energy is mainly spent on the new version. However, If you provide a stable reproducible example, I will try to fix this bug.
Remember to open a new issure.