-
Notifications
You must be signed in to change notification settings - Fork 715
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
sanity.system MauveMultiThreadLoadTest DateFormat.equals #8204
Comments
https://ci.eclipse.org/openj9/job/Test_openjdk11_j9_sanity.system_ppc64_aix_Nightly/325 |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_ppc64le_linux_xl_Personal_mauveLoadTest/4 |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_linux_Nightly_mauveLoadTest/28 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_mac_Nightly_mauveLoadTest/28 |
https://ci.eclipse.org/openj9/job/Test_openjdk14_j9_sanity.system_ppc64_aix_Nightly/96 |
Tentatively assigning to jit given the intermittent nature of the failure. |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_mac_Nightly_mauveLoadTest/31 |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_mac_Nightly_mauveLoadTest/32 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_s390x_linux_xl_Personal_mauveLoadTest/5 |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_sanity.system_ppc64_aix_Nightly/406 |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_ppc64_aix_Nightly_mauveLoadTest/40 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_linux_Nightly_mauveLoadTest/35 |
@JasonFengJ9 Can you take a look at the test and validate it's valid to run this in the multi-threaded way it's being run? |
@DanHeidinga The investigation shows that it is not valid to run this test in the multi-threaded way as per following details. The test code snippet in question are:
If there is another thread calling
So this test is problematic in a multi-thread environment in which current JVM default Locale could be changed by another thread. fyi @Mesbah-Alam |
@JasonFengJ9 thanks for tracking this down.
The test is run in the OpenJ9 jenkins so the source should be accessible and modifyable. @Mesbah-Alam can you link to the source for this test? |
The test is part of the third party Mauve test package that gets downloaded from its public site by the systemtest.getDependency build. So, I think instead of trying to modify the source, we should exclude this test from the multi-threaded target and only run it in the single-threaded target-- as @JasonFengJ9 suggested above. |
It appears the As per https://www.sourceware.org/mauve/contribute.html, Contributing to Mauve is fairly easy -- we're pretty generous with cvs write access, and we don't require copyright assignments or anything like that. We do ask that you not have read Sun's source; we are a cleanroom test suite and we're concerned about contamination. fyi @smlambert @llxia |
I don't think we have ever tried to commit anything to the mauve project before, so I am not aware of the process and how long they take to respond / merge a change log entry (that's the format in which they expect a contribution to be). It'd be a good thing to try. @JasonFengJ9 - Do you already have the change to submit? |
I don't think there is anything wrong with the tests, it's the way we're running them multi-threaded which is causing the problems. Some of the tests have side affects which affect other tests. We've had similar problems before with other mauve tests. From the multi-threaded testing, we should either exclude all the tests which call Locale.setDefault(), or all the tests that use the default locale. I expect the former is easier to track down, Jason already provided the list. |
Following code snippet with specified local can work in a multi-threaded environment.
In current Mauve test, the two
With the proposed modification, following two
I agree that excluding the tests or running in single thread mode is an easier workaround. |
I agree. I will create a PR for the excludes. |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_ppc64le_linux_Release_mauveLoadTest/2 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-32_windows_Release_mauveLoadTest/2 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_ppc64le_linux_xl_Personal_mauveLoadTest/6 |
Another failure The test is now running with @Mesbah-Alam can we trigger a core dump when one of the tests fail? |
I don't expect this will get resolved in the 0.21 release, moving forward. |
https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_ppc64_aix_Nightly_mauveLoadTest/59 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_linux_Nightly_mauveLoadTest/54 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_x86-64_mac_Nightly_mauveLoadTest/55 https://ci.eclipse.org/openj9/job/Test_openjdk8_j9_special.system_ppc64_aix_Release_mauveLoadTest/4 |
@Mesbah-Alam the place where adoptium/STF#82 throws MauveTestFailureException isn't when the failure occurs, it's when the failures are reported. The exception needs to happen at the time the first failure is discovered, and the DateFormat objects are on the stack. Stack:
|
Do you want the exception to be added in the Mauve test code itself? This means we'd have to request the change to be added by the Mauve proect. |
Is there no other way to hook into the failure? Do you have a link to the test code which is causing the failure and now it handles the failure. |
Throwing of the exception was added into the STF wrapper that runs Mauve test loads, here: https://github.com/AdoptOpenJDK/stf/blob/ad1a2360c73ec638ccedc45abbc80d75f6ee2001/stf.load/src/stf.load/net/adoptopenjdk/loadTest/LoadTestRunner.java#L289 |
I'm not looking for a link to what was added and doesn't work, I'm looking for a link to the actual test which is failing, and how the framework handles that failure. |
The test class that fails is The source can be seen by downloading The framework code may be found under |
See here where the MauveAdaptor creates an instance of SingleTestHarness. What you need to do is create a subclass of gnu.testlet.SingleTestHarness, and change the MauveAdaptor to create an instance of this subclass and use it instead. In the subclass override the implementation of debug(String). It can call super.debug(String), and then throw the MauveTestFailureException. You can catch the exception (https://github.com/AdoptOpenJDK/stf/tree/master/stf.load/src/stf.load/net/adoptopenjdk/loadTest/adaptors/MauveAdaptor.java#L75) and proceed. |
Actually, creating the subclass of SingleTestHarness isn't required. You can check the testResult at the following location and throw the MauveTestFailureException. |
@Mesbah-Alam here is another failure, but we didn't get any core dump. The -Xdump command adoptium/aqa-systemtest#356 doesn't specify the correct package name. It specifies |
MauveTestFailureException class was refactored into a new package in https://github.com/AdoptOpenJDK/stf/pull/83/files#diff-ad5f317c88f122ae7ecf2be60b18f61e , but the reference was not updated in https://github.com/AdoptOpenJDK/openjdk-systemtest/pull/356/files#diff-a7b21278a26c58dc97240147769b0135 - my mistake. Fixing it now. |
We got a core! I located the two objects being compared and looked at them manually. The difference is in the calendar TimeZone objects. One has GMT and the other Etc/UTC. So I think the problem is not with something calling Locale.setDefault() but calling TimeZone.setDefault(). @Mesbah-Alam can you please check the other mauve tests for callers of TimeZone.setDefault(). |
Fixed via adoptium/aqa-systemtest#358 |
https://ci.eclipse.org/openj9/job/Test_openjdk11_j9_sanity.system_ppc64_aix_Release/21
MauveMultiThreadLoadTest_0
The text was updated successfully, but these errors were encountered: