[Urgent] Concurrency issue when a @Async method is first time accessed
Closed this issue · 9 comments
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!
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
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);
}
}
I will release new version to fix it today.
I think changing callSites from HashMap to ConcurrentHashMap fix this issue, too.
Thanks. ConcurrentHashMap works as well.
Have released. It might take a while for you to find it in maven centeral repo.
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?
@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.
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
- 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));
- 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.
@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.