From 63d54d756d2b1e28d8557ea89bc62610b3eb013a Mon Sep 17 00:00:00 2001 From: gabilang Date: Fri, 10 Nov 2023 14:38:12 +0530 Subject: [PATCH 1/6] Fix default worker crash with error return --- .../compiler/bir/codegen/JvmErrorGen.java | 5 +++-- .../bir/codegen/JvmTerminatorGen.java | 19 ++++++++++--------- .../bir/codegen/interop/InteropMethodGen.java | 2 +- .../bir/codegen/methodgen/MethodGen.java | 4 ++-- 4 files changed, 16 insertions(+), 14 deletions(-) diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmErrorGen.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmErrorGen.java index 5d0cbc0a2304..c0fb9d134a49 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmErrorGen.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmErrorGen.java @@ -80,7 +80,8 @@ void genPanic(BIRTerminator.Panic panicTerm) { } public void generateTryCatch(BIRNode.BIRFunction func, String funcName, BIRNode.BIRBasicBlock currentBB, - JvmTerminatorGen termGen, LabelGenerator labelGen, int invocationVarIndex) { + JvmTerminatorGen termGen, LabelGenerator labelGen, int invocationVarIndex, + int localVarOffset) { BIRNode.BIRErrorEntry currentEE = findErrorEntry(func.errorTable, currentBB); if (currentEE == null) { @@ -108,7 +109,7 @@ public void generateTryCatch(BIRNode.BIRFunction func, String funcName, BIRNode. this.mv.visitMethodInsn(INVOKESTATIC, ERROR_UTILS, CREATE_INTEROP_ERROR_METHOD, CREATE_ERROR_FROM_THROWABLE, false); jvmInstructionGen.generateVarStore(this.mv, retVarDcl, retIndex); - termGen.genReturnTerm(retIndex, func, invocationVarIndex); + termGen.genReturnTerm(retIndex, func, invocationVarIndex, localVarOffset); this.mv.visitJumpInsn(GOTO, jumpLabel); } if (!exeptionExist) { diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTerminatorGen.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTerminatorGen.java index d8cb660f1c77..aebbcd6c0f99 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTerminatorGen.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/JvmTerminatorGen.java @@ -364,7 +364,7 @@ public void genTerminator(BIRTerminator terminator, String moduleClassName, BIRN this.genBranchTerm((BIRTerminator.Branch) terminator, funcName); return; case RETURN: - this.genReturnTerm(returnVarRefIndex, func, invocationVarIndex); + this.genReturnTerm(returnVarRefIndex, func, invocationVarIndex, localVarOffset); return; case PANIC: this.errorGen.genPanic((BIRTerminator.Panic) terminator); @@ -448,9 +448,9 @@ private void genUnlockTerm(BIRTerminator.Unlock unlockIns, String funcName) { } private void handleErrorRetInUnion(int returnVarRefIndex, List channels, BUnionType bType, - int invocationVarIndex) { + int invocationVarIndex, int localVarOffset) { - if (channels.size() == 0) { + if (channels.isEmpty()) { return; } @@ -465,7 +465,7 @@ private void handleErrorRetInUnion(int returnVarRefIndex, List channels, int retIndex, int invocationVarIndex) { - if (channels.size() == 0) { + if (channels.isEmpty()) { return; } @@ -1327,13 +1327,14 @@ private void genResourcePathArgs(List pathArgs) { mv.visitVarInsn(ALOAD, bundledArrayIndex); } - public void genReturnTerm(int returnVarRefIndex, BIRNode.BIRFunction func, int invocationVarIndex) { + public void genReturnTerm(int returnVarRefIndex, BIRNode.BIRFunction func, int invocationVarIndex, + int localVarOffset) { BType bType = unifier.build(func.type.retType); - generateReturnTermFromType(returnVarRefIndex, bType, func, invocationVarIndex); + generateReturnTermFromType(returnVarRefIndex, bType, func, invocationVarIndex, localVarOffset); } private void generateReturnTermFromType(int returnVarRefIndex, BType bType, BIRNode.BIRFunction func, - int invocationVarIndex) { + int invocationVarIndex, int localVarOffset) { bType = JvmCodeGenUtil.getImpliedType(bType); if (TypeTags.isIntegerTypeTag(bType.tag)) { this.mv.visitVarInsn(LLOAD, returnVarRefIndex); @@ -1380,7 +1381,7 @@ private void generateReturnTermFromType(int returnVarRefIndex, BType bType, BIRN break; case TypeTags.UNION: this.handleErrorRetInUnion(returnVarRefIndex, Arrays.asList(func.workerChannels), - (BUnionType) bType, invocationVarIndex); + (BUnionType) bType, invocationVarIndex, localVarOffset); this.mv.visitVarInsn(ALOAD, returnVarRefIndex); this.mv.visitInsn(ARETURN); break; diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/interop/InteropMethodGen.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/interop/InteropMethodGen.java index 35108c9113ab..6d92a296b5a6 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/interop/InteropMethodGen.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/interop/InteropMethodGen.java @@ -241,7 +241,7 @@ static void genJFieldForInteropField(JFieldBIRFunction birFunc, ClassWriter clas Label retLabel = labelGen.getLabel("return_lable"); mv.visitLabel(retLabel); mv.visitLineNumber(birFunc.pos.lineRange().endLine().line() + 1, retLabel); - termGen.genReturnTerm(returnVarRefIndex, birFunc, -1); + termGen.genReturnTerm(returnVarRefIndex, birFunc, -1, 0); JvmCodeGenUtil.visitMaxStackForMethod(mv, birFunc.name.value, birFunc.javaField.getDeclaringClassName()); mv.visitEnd(); } diff --git a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/methodgen/MethodGen.java b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/methodgen/MethodGen.java index 93f966a85851..82eb270ee68f 100644 --- a/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/methodgen/MethodGen.java +++ b/compiler/ballerina-lang/src/main/java/org/wso2/ballerinalang/compiler/bir/codegen/methodgen/MethodGen.java @@ -308,7 +308,7 @@ public void genJMethodForBFunc(BIRFunction func, ClassWriter cw, BIRPackage modu Label methodEndLabel = new Label(); mv.visitLabel(methodEndLabel); - termGen.genReturnTerm(returnVarRefIndex, func, invocationVarIndex); + termGen.genReturnTerm(returnVarRefIndex, func, invocationVarIndex, localVarOffset); // Create Local Variable Table createLocalVariableTable(func, indexMap, localVarOffset, mv, methodStartLabel, labelGen, methodEndLabel); @@ -570,7 +570,7 @@ void generateBasicBlocks(MethodVisitor mv, LabelGenerator labelGen, JvmErrorGen lastScope = JvmCodeGenUtil .getLastScopeFromTerminator(mv, bb, funcName, labelGen, lastScope, visitedScopesSet); - errorGen.generateTryCatch(func, funcName, bb, termGen, labelGen, invocationVarIndex); + errorGen.generateTryCatch(func, funcName, bb, termGen, labelGen, invocationVarIndex, localVarOffset); String yieldStatus = getYieldStatusByTerminator(terminator); From 0312066fa176e358fc0d333cb75e24b127338da4 Mon Sep 17 00:00:00 2001 From: gabilang Date: Sun, 12 Nov 2023 00:59:09 +0530 Subject: [PATCH 2/6] Add unit test for service with error union return --- .../nativeimpl/jvm/tests/MockListener.java | 15 +++++ .../test/services/ErrorReturnTest.java | 7 ++ ...error_union_return_with_default_worker.bal | 66 +++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java index c33d01036773..a96bddb124ce 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java @@ -22,9 +22,13 @@ import io.ballerina.runtime.api.Runtime; import io.ballerina.runtime.api.async.Callback; import io.ballerina.runtime.api.values.BError; +import io.ballerina.runtime.api.values.BFuture; import io.ballerina.runtime.api.values.BObject; import io.ballerina.runtime.api.values.BString; +import io.ballerina.runtime.internal.types.BUnionType; +import java.util.ArrayList; +import java.util.List; import java.util.concurrent.CountDownLatch; /** @@ -34,6 +38,7 @@ public class MockListener { private static BObject service; private static BError err; + private static BFuture result; public static Object attach(BObject servObj) { service = servObj; @@ -61,4 +66,14 @@ public void notifyFailure(BError error) { } return err; } + + public static BFuture invokeResourceWithUnionReturn(Environment env, BString name) { + if (service != null) { + Runtime runtime = env.getRuntime(); + result = runtime.invokeMethodAsyncConcurrently(service, name.getValue(), null, null, null, null, + new BUnionType(new ArrayList<>(List.of(PredefinedTypes.TYPE_INT, + PredefinedTypes.TYPE_ERROR)))); + } + return result; + } } diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/services/ErrorReturnTest.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/services/ErrorReturnTest.java index d54e976131e2..e2dd52999198 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/services/ErrorReturnTest.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/test/services/ErrorReturnTest.java @@ -33,4 +33,11 @@ public void testErrorReturnFunction() { CompileResult compileResult = BCompileUtil.compile("test-src/services/error_return_function.bal"); BRunUtil.invoke(compileResult, "testErrorFunction"); } + + @Test(description = "Tests invoking a function with default worker returning an error union value in a service") + public void testErrorReturnFunctionWithDistinctListenerArg() { + CompileResult compileResult = BCompileUtil.compile( + "test-src/services/error_union_return_with_default_worker.bal"); + BRunUtil.invoke(compileResult, "testErrorUnionWithDefaultWorkerFunction"); + } } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal new file mode 100644 index 000000000000..8ddf61d33217 --- /dev/null +++ b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal @@ -0,0 +1,66 @@ +// Copyright (c) 2023, WSO2 LLC. (https://www.wso2.com) All Rights Reserved. +// +// WSO2 LLC. licenses this file to you under the Apache License, +// Version 2.0 (the "License"); you may not use this file except +// in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +import ballerina/jballerina.java; +import ballerina/test; + +public class Listener { + public isolated function 'start() returns error? { + } + public isolated function gracefulStop() returns error? { + } + public isolated function immediateStop() returns error? { + } + public isolated function detach(service object {} s) returns error? { + } + public isolated function attach(service object {} s, string[]|string? name = ()) returns error? { + return self.register(s, name); + } + isolated function register(service object {} s, string[]|string? name) returns error? { + return externAttach(s); + } +} + +isolated function externAttach(service object {} s) returns error? = @java:Method { + 'class: "org.ballerinalang.nativeimpl.jvm.tests.MockListener", + name: "attach" +} external; + +listener Listener lsn = new(); + +service / on lsn { + resource function get number() returns int|error { + worker w1 returns error? { + int a = 10; + a -> function; + } + int b = <- w1; + return b; + } +} + +function invokeResource(string name) returns future = @java:Method { + 'class: "org.ballerinalang.nativeimpl.jvm.tests.MockListener", + name: "invokeResourceWithUnionReturn" +} external; + +public function testErrorUnionWithDefaultWorkerFunction() { + future num = invokeResource("$get$number"); + int|error res = wait num; + if (res is int) { + test:assertEquals(10, res); + } +} From 18bfa6119bf829f58a253f9a370f5fc459128d82 Mon Sep 17 00:00:00 2001 From: gabilang Date: Tue, 21 Nov 2023 12:50:57 +0530 Subject: [PATCH 3/6] Address review suggestions --- .../nativeimpl/jvm/tests/MockListener.java | 12 ++++-------- ...error_union_return_with_default_worker.bal | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java index a96bddb124ce..26d87fb3bda7 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java @@ -38,7 +38,6 @@ public class MockListener { private static BObject service; private static BError err; - private static BFuture result; public static Object attach(BObject servObj) { service = servObj; @@ -68,12 +67,9 @@ public void notifyFailure(BError error) { } public static BFuture invokeResourceWithUnionReturn(Environment env, BString name) { - if (service != null) { - Runtime runtime = env.getRuntime(); - result = runtime.invokeMethodAsyncConcurrently(service, name.getValue(), null, null, null, null, - new BUnionType(new ArrayList<>(List.of(PredefinedTypes.TYPE_INT, - PredefinedTypes.TYPE_ERROR)))); - } - return result; + Runtime runtime = env.getRuntime(); + return runtime.invokeMethodAsyncConcurrently(service, name.getValue(), null, null, null, null, + new BUnionType(new ArrayList<>(List.of(PredefinedTypes.TYPE_INT, + PredefinedTypes.TYPE_ERROR)))); } } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal index 8ddf61d33217..342cdf6503ad 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal @@ -63,4 +63,23 @@ public function testErrorUnionWithDefaultWorkerFunction() { if (res is int) { test:assertEquals(10, res); } + + Class c = new(10); + test:assertEquals(10, checkpanic c.getId()); +} + +public class Class { + private int id; + + public function init(int id) { + self.id = id; + } + + public function getId() returns int|error { + worker w1 returns error? { + self.id -> function; + + } + return <- w1; + } } From 908dd8b35f4038ed4de144b02b7890fc72d52117 Mon Sep 17 00:00:00 2001 From: gabilang Date: Tue, 21 Nov 2023 13:13:47 +0530 Subject: [PATCH 4/6] Remove additional white line --- .../test-src/services/error_union_return_with_default_worker.bal | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal index 342cdf6503ad..66223abdaea1 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal @@ -78,7 +78,6 @@ public class Class { public function getId() returns int|error { worker w1 returns error? { self.id -> function; - } return <- w1; } From d8c385cc85dea7ae6f038e94880c8758251769d5 Mon Sep 17 00:00:00 2001 From: gabilang Date: Tue, 21 Nov 2023 13:31:18 +0530 Subject: [PATCH 5/6] Update license header --- .../services/error_union_return_with_default_worker.bal | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal index 66223abdaea1..fe40aa9565d3 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal @@ -1,4 +1,4 @@ -// Copyright (c) 2023, WSO2 LLC. (https://www.wso2.com) All Rights Reserved. +// Copyright (c) 2023, WSO2 LLC. (https://www.wso2.com). // // WSO2 LLC. licenses this file to you under the Apache License, // Version 2.0 (the "License"); you may not use this file except From fbffe4436feda40d19886d56e7e74d65ca82f612 Mon Sep 17 00:00:00 2001 From: gabilang Date: Tue, 21 Nov 2023 18:58:21 +0530 Subject: [PATCH 6/6] Remove the added service test case --- .../nativeimpl/jvm/tests/MockListener.java | 11 ----- ...error_union_return_with_default_worker.bal | 47 ------------------- 2 files changed, 58 deletions(-) diff --git a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java index 26d87fb3bda7..c33d01036773 100644 --- a/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java +++ b/tests/jballerina-unit-test/src/test/java/org/ballerinalang/nativeimpl/jvm/tests/MockListener.java @@ -22,13 +22,9 @@ import io.ballerina.runtime.api.Runtime; import io.ballerina.runtime.api.async.Callback; import io.ballerina.runtime.api.values.BError; -import io.ballerina.runtime.api.values.BFuture; import io.ballerina.runtime.api.values.BObject; import io.ballerina.runtime.api.values.BString; -import io.ballerina.runtime.internal.types.BUnionType; -import java.util.ArrayList; -import java.util.List; import java.util.concurrent.CountDownLatch; /** @@ -65,11 +61,4 @@ public void notifyFailure(BError error) { } return err; } - - public static BFuture invokeResourceWithUnionReturn(Environment env, BString name) { - Runtime runtime = env.getRuntime(); - return runtime.invokeMethodAsyncConcurrently(service, name.getValue(), null, null, null, null, - new BUnionType(new ArrayList<>(List.of(PredefinedTypes.TYPE_INT, - PredefinedTypes.TYPE_ERROR)))); - } } diff --git a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal index fe40aa9565d3..f6e4d94179f6 100644 --- a/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal +++ b/tests/jballerina-unit-test/src/test/resources/test-src/services/error_union_return_with_default_worker.bal @@ -14,56 +14,9 @@ // specific language governing permissions and limitations // under the License. -import ballerina/jballerina.java; import ballerina/test; -public class Listener { - public isolated function 'start() returns error? { - } - public isolated function gracefulStop() returns error? { - } - public isolated function immediateStop() returns error? { - } - public isolated function detach(service object {} s) returns error? { - } - public isolated function attach(service object {} s, string[]|string? name = ()) returns error? { - return self.register(s, name); - } - isolated function register(service object {} s, string[]|string? name) returns error? { - return externAttach(s); - } -} - -isolated function externAttach(service object {} s) returns error? = @java:Method { - 'class: "org.ballerinalang.nativeimpl.jvm.tests.MockListener", - name: "attach" -} external; - -listener Listener lsn = new(); - -service / on lsn { - resource function get number() returns int|error { - worker w1 returns error? { - int a = 10; - a -> function; - } - int b = <- w1; - return b; - } -} - -function invokeResource(string name) returns future = @java:Method { - 'class: "org.ballerinalang.nativeimpl.jvm.tests.MockListener", - name: "invokeResourceWithUnionReturn" -} external; - public function testErrorUnionWithDefaultWorkerFunction() { - future num = invokeResource("$get$number"); - int|error res = wait num; - if (res is int) { - test:assertEquals(10, res); - } - Class c = new(10); test:assertEquals(10, checkpanic c.getId()); }