vipcxj / jasync

make async-await code style available in java just like csharp and es6
Apache License 2.0
129 stars 14 forks source link

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

Closed knuclechan closed 2 years ago

knuclechan commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

knuclechan commented 2 years ago

Thanks. ConcurrentHashMap works as well.

vipcxj commented 2 years ago

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

knuclechan commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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));
  1. 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 commented 2 years ago

@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.