bytedeco / javacpp

The missing bridge between Java and native C++
Other
4.43k stars 576 forks source link

Updated Generator.java to fix the flaky errors #727

Open njain2208 opened 8 months ago

njain2208 commented 8 months ago

PR Overview:


This PR fixes the flaky/non-deterministic behavior of the following test because it assumes the ordering of methods returned by getDeclaredMethods()

org.bytedeco.javacpp.BufferTest

org.bytedeco.javacpp.EnumTest

org.bytedeco.javacpp.PointerTest

Test Overview:


In all of the above tests, the tests call the setUpClass() method where the problem arises. The setUpClass() makes an internal to the Generator class’s object where the problem resides.

You can reproduce the issue by running the following commands (the below command is for BufferTest, please change class name for targeting other tests) -

mvn install -pl . -am -DskipTests
mvn test  -Dtest=org.bytedeco.javacpp.BufferTest
mvn edu.illinois:nondex-maven-plugin:2.1.1:nondex -Dtest=org.bytedeco.javacpp.BufferTest

Root Cause


Method[] methods = cls.getDeclaredMethods();

In the Generator class, the cls.getDeclaredMethods() method returns an array with values in a non-deterministic order, leading to the core of the issue. Within the functionMethods() method of the Generator class, there's a call to cls.getDeclaredMethods() used to populate the callbackAllocators[] array. Additionally, the same cls.getDeclaredMethods() method is invoked in the methods() method of the Generator class, which utilizes the callbackAllocators array. However, due to the differing order of results produced by cls.getDeclaredMethods(), it results in a test failure. This flakiness was identified by the nondex tool created by the researchers of UIUC

Fix:


To fix the issue I decided to make the output of callbackAllocators array deterministic by ordering the cls.getDeclaredMethods() result both in methods() method and functionMethods() method.

https://github.com/njain2208/javacpp/blob/4c62303548be1b9edc3e22d23d3674c397ff9829/src/main/java/org/bytedeco/javacpp/tools/Generator.java#L2085-L2091

https://github.com/njain2208/javacpp/blob/4c62303548be1b9edc3e22d23d3674c397ff9829/src/main/java/org/bytedeco/javacpp/tools/Generator.java#L3643-L3649

saudet commented 7 months ago

I've never encountered any problems with that, and I will not merge something like this that needs sorting, but I would be inclined to merge a solution that calls getDeclaredMethods() only once, if you can provide it.