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

fix flaky test testObserverMethodsInParentOfAlternativeAndSpecialized #100

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ljmcr
Copy link

@ljmcr ljmcr commented Sep 4, 2024

…Beans

@ljmcr ljmcr closed this Sep 4, 2024
@ljmcr ljmcr reopened this Oct 4, 2024
@ljmcr
Copy link
Author

ljmcr commented Oct 4, 2024

The error was caused when Running the following command by setting random seed to 10:
mvn -pl webbeans-impl nondex-maven-plugin:2.1.7:nondex -Dtest=org.apache.webbeans.test.specalization.observer.pub.PublicObserverTest#testObserverMethodsInParentOfAlternativeAndSpecializedBeans -DnondexRuns=10

Original error:
365017594-efa57e91-ffd3-4017-9b21-6a066b1fce25

error stack trace:
365017667-9893c938-6d61-4e2b-b008-430526ad16d3

Specific line of code: Line 172 at SpecializationUtil.java:
365018047-1d260fed-a460-42f6-83d3-bbba684cdc01

Adding a hashset to store all the elements that should be remove in beanAttributeMap, and remove all those elements in the set together after looping through the whole map. This will avoid removing elements while looping with iterator, which may cause IndexOutOfBoundException or ConcurrentModificationException when doing parallel programming .

Copy link
Member

@struberg struberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code part is not intended to be used in parallel. When running the Tests in parallel, they should boot up different BeanManagers. That can be done with running those in own isolated ClassLoaders. I also tried it locally with even more nondex runs and got no problems.

@ljmcr
Copy link
Author

ljmcr commented Oct 16, 2024

This code part is not intended to be used in parallel. When running the Tests in parallel, they should boot up different BeanManagers. That can be done with running those in own isolated ClassLoaders. I also tried it locally with even more nondex runs and got no problems.

Hi, Thank you for the response.
The test failure occurs when running with different seed values using Nondex, not when running a single Nondex run multiple times. Could you try reproducing the error using the following command?
Dtest=org.apache.webbeans.test.specalization.observer.pub.PublicObserverTest#testObserverMethodsInParentOfAlternativeAndSpecializedBeans -DnondexRuns=10
which -DnondexRuns=10 flag represent running with different seed values. For more information of Nondex may check NonDex

@struberg
Copy link
Member

Hi, Thank you for the response. The test failure occurs when running with different seed values using Nondex, not when running a single Nondex run multiple times.

Hi!

Seems nondex is a really cool tool for testing whether tests are really reliable. In this very case it creates a false positive though. The reason is that the whole CDI container is not intended to be started in parallel for the same application. Or rather for the same ClassLoader. This is the very core of a JavaEE/JakartaEE container and thus most parts are single threaded by definition. Probably there is some room for internal parallelisation but I fear removing from a list based on a single boolean is not helping much.
The point is that OWB (as Spring and tons of other containers) does rely on basically a Map<ClassLoader, BeanManageImpl>. You CAN support running parallel tests, but then you have to do tricks like adding an intermediate ClassLoader (new URLClassLoader) like we did in a few specific tests and set this as TCCL for the thread.

@ljmcr
Copy link
Author

ljmcr commented Oct 23, 2024

Hi, Thank you for the reply,
Please ignore the previous mention of "parallel", which is not a correct analysis in this case. The issue is come from modifying the collection during iteration caused an ArrayIndexOutOfBoundsException, as the internal state of the IdentityHashMap (or other underlying data structures) became inconsistent when the iteration order changed unexpectedly. This error occurred because the original code implicitly relied on the collection being in a particular state while modifying it during iteration, an assumption that NonDex exposed as a bug. The original removeIf() method modified the collection during iteration, which is inherently risky when the collection's internal state may change (especially under NonDex). I have also tried to use the Iterator() for map as an alternate solution, but the errors still occurred when running with different seeds for multiple times. Using a hashset to collect all the elements to be removed is a better solution from my point of view, which ignore the dependency on iteration order and avoids modifying the map during iteration, also ensures that the code can handle randomized iteration order without throwing exceptions or causing inconsistent internal states.
I am willing to discuss any other solutions and appreciate for any further feedbacks, thank you!

Hi, Thank you for the response. The test failure occurs when running with different seed values using Nondex, not when running a single Nondex run multiple times.

Hi!

Seems nondex is a really cool tool for testing whether tests are really reliable. In this very case it creates a false positive though. The reason is that the whole CDI container is not intended to be started in parallel for the same application. Or rather for the same ClassLoader. This is the very core of a JavaEE/JakartaEE container and thus most parts are single threaded by definition. Probably there is some room for internal parallelisation but I fear removing from a list based on a single boolean is not helping much. The point is that OWB (as Spring and tons of other containers) does rely on basically a Map<ClassLoader, BeanManageImpl>. You CAN support running parallel tests, but then you have to do tricks like adding an intermediate ClassLoader (new URLClassLoader) like we did in a few specific tests and set this as TCCL for the thread.

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

Successfully merging this pull request may close these issues.

2 participants