Fix random failing ServiceSupplierTestCase.testOptionalReferences #488 #829
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ipse-platform#488 The ServiceSupplierTestCase.testOptionalReferences test case randomly fails. The test aims to wait for services to be enabled or disabled, but it only waits until a single value is changed although it asserts that multiple values are changed afterwards. This is a race condition, as the order in which the values are changed or in which the changes are visible is non-deterministic, also due to missing memory barriers. This change addresses this issue via two means: 1. it makes the fields of the asserted values volatile to ensure that changed values eventually become visible to the UI thread 2. it waits for all values to be changed before asserting their values to allow different orders of value changes
jukzi
reviewed
Nov 29, 2023
| @@ -73,11 +73,11 @@ public static class TestDisabledBean { | |||
| @Inject | |||
| @Optional | |||
| @Service(filterExpression = "(component=disabled)") | |||
| TestService disabledService; | |||
| volatile TestService disabledService; | |||
There was a problem hiding this comment.
if this Lists are used concurrently its not enough to use volatile, they need to be Concurrent Lists, or synchronized
There was a problem hiding this comment.
True. The volatile was an attempt to introduce a memory barrier to ensure that changed values become eventually visible, but, of course, that does not apply to content changes of the list.
There was a problem hiding this comment.
Since the field is injected, one cannot easily influence the type of list, i.e., ensure that it is thread-safe. But gladly OSGi handles that case:
By far the easiest method is to use field injection of a list of services. If you make this field volatile then DS will inject a new list whenever the set of tracked services changes.
See https://enroute.osgi.org/FAQ/300-declarative-services.html#tracking-multiple-services
So from my understanding, volatile should be correct here.
HeikoKlare
added a commit
to HeikoKlare/eclipse.platform
that referenced
this pull request
Nov 29, 2023
…ipse-platform#488 Fixes two issues: 1. The ServiceSupplierTestCase asserts several values that are assigned by an asynchronous task. To this end, it waits for the conditions to be fulfilled by the asynchronous task. However, the conditions are incomplete, leading to a race condition. 2. The ServiceSupplierTestCase has been duplicated for javax and jakarta and a fix for a random failure has only been applied to a single implementation in eclipse-platform#829 This change corrects the conditions the test waits for and aligns both copies of the test class with each other. Fixes eclipse-platform#488
HeikoKlare
added a commit
that referenced
this pull request
Nov 29, 2023
Fixes two issues: 1. The ServiceSupplierTestCase asserts several values that are assigned by an asynchronous task. To this end, it waits for the conditions to be fulfilled by the asynchronous task. However, the conditions are incomplete, leading to a race condition. 2. The ServiceSupplierTestCase has been duplicated for javax and jakarta and a fix for a random failure has only been applied to a single implementation in #829 This change corrects the conditions the test waits for and aligns both copies of the test class with each other. Fixes #488
Michael5601
pushed a commit
to CodeLtDave/eclipse.platform
that referenced
this pull request
Feb 12, 2024
…ipse-platform#488 Fixes two issues: 1. The ServiceSupplierTestCase asserts several values that are assigned by an asynchronous task. To this end, it waits for the conditions to be fulfilled by the asynchronous task. However, the conditions are incomplete, leading to a race condition. 2. The ServiceSupplierTestCase has been duplicated for javax and jakarta and a fix for a random failure has only been applied to a single implementation in eclipse-platform#829 This change corrects the conditions the test waits for and aligns both copies of the test class with each other. Fixes eclipse-platform#488
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
ServiceSupplierTestCase.testOptionalReferencestest case randomly fails. The test aims to wait for services to be enabled or disabled, but it only waits until a single value is changed although it asserts that multiple values are changed afterwards. This is a race condition, as the order in which the values are changed or in which the changes are visible is non-deterministic, also due to missing memory barriers.This change addresses this issue via two means: