Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove non-determinism in BeanCacheKey.java to fix flaky test BeanCacheKeyUnitTest.testEquals2Annotations #53

Closed
wants to merge 1 commit into from

Conversation

asha-boyapati
Copy link

@asha-boyapati asha-boyapati commented Jun 27, 2023

Description

The test

org.apache.webbeans.test.annotation.binding.BeanCacheKeyUnitTest.testEquals2Annotations

fails under environment NonDex which detects flakiness under non-deterministic usages.

The potential problem is that the order of Methods returned by GetDeclaredMethods (reference) is not deterministic.

Quote from Oracle Java 8 Doc:

The elements in the returned array are not sorted and are not in any particular order.

Reproduce

mvn edu.illinois:nondex-maven-plugin:1.1.2:nondex \
    -pl  webbeans-impl \
    -Dtest=org.apache.webbeans.test.annotation.binding.BeanCacheKeyUnitTest#testEquals2Annotations

Please see the following Continuous Integration log that shows the flakiness:
https://github.com/asha-boyapati/openwebbeans/actions/runs/5041879894

This PR fixes the flaky test by removing the non-determinism in BeanCacheKey.java.

Please see the following Continuous Integration log that shows that the flakiness is fixed by this change:
https://github.com/asha-boyapati/openwebbeans/actions/runs/5041912235

@rmannibucau
Copy link
Contributor

Hi,

AFAIK this is not needed nor flaky because it is stable per classloader so stable in the OWB context so it sounds faster to not sort+toString the methods rather than doing it to not gain any determinism.

Didnt check the failure details but do you have them cause by jvm contract it shouldnt be possible.

@rmannibucau
Copy link
Contributor

Ok, found the bug you speak about, it is in your code in https://github.com/TestingResearchIllinois/NonDex/blob/master/nondex-instrumentation/src/main/java/edu/illinois/nondex/instr/ClassVisitorShufflingAdder.java#L39 so will close the PR since it does not fix any actual flakiness.

@asha-boyapati
Copy link
Author

Thank you for your response.

If you don't mind, I have a question about your comment. The specification of GetDeclaredMethods says that the returned elements are not in any particular order.

If I understand your comment, you are saying that given any classloader, the order of the returned elements is always deterministic. Is this true only of OWB classloader or also the standard Java classloader? Is there some documentation about this you can point me to?

Thanks again!

@rmannibucau
Copy link
Contributor

this is the standard JVM behavior, Class cached the loaded bytecode which has no deterministic order but once loaded it is sorted. Not sure it is well documented but all JVM behave like that.

@asha-boyapati
Copy link
Author

For completeness, I want to add that the NonDex tool supports multiple modes.

In the default mode, -DNondexMode=FULL, this test fails as flaky.

In the other mode, -DNondexMode=ONE, this test does not fail.

The other mode only checks for some sources of nondeterminism (such as the iteration order of a HashSet), but not others (such as the order of elements returned by GetDeclaredMethods).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants