You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "MoritzArena (via GitHub)" <gi...@apache.org> on 2025/09/01 10:47:21 UTC
[PR] fix mutiny race condition issue [dubbo]
MoritzArena opened a new pull request, #15662:
URL: https://github.com/apache/dubbo/pull/15662
## Problem Description
Ref: #15660
The `MutinyClientCallsTest.testOneToManyReturnsMultiAndEmitsItems()` test case encounters a `NullPointerException` in certain environments:
```
Exception in thread "Thread-2" java.lang.NullPointerException:
Cannot invoke "java.util.concurrent.Flow$Subscriber.onNext(Object)" because "this.downstream" is null
at org.apache.dubbo.mutiny.AbstractTripleMutinyPublisher.onNext(AbstractTripleMutinyPublisher.java:126)
```
This error occurs when the test attempts to call `publisher.onNext()` before the downstream subscriber is properly established, resulting in a null `downstream` reference.
## Root Cause Analysis
### Why Local Tests Pass
In local development environments, tests typically pass due to:
- **Sufficient CPU resources**: Local machines usually have adequate processing power
- **Lower system load**: Fewer competing processes and background tasks
- **Predictable timing**: Consistent thread scheduling allows the subscription to complete before data emission
- **JVM optimizations**: Local JVMs may have different optimization profiles
### Why GitHub CI Fails Intermittently
GitHub CI environments are prone to this race condition because of:
- **Resource constraints**: Shared CPU resources with limited allocation per container
- **Variable system load**: Multiple builds and processes running concurrently
- **Virtualization overhead**: Additional scheduling delays in containerized environments
- **Thread scheduling variance**: Less predictable timing due to resource competition
The core issue is a **race condition** where the data-emitting thread starts before the `Multi` subscription is fully established.
## Solution Comparison
### Approach 1: Original Code (Problematic)
```java
publisher.onSubscribe(fakeSubscription);
new Thread(() -> {
publisher.onNext("item1");
publisher.onNext("item2");
publisher.onCompleted();
}).start();
```
**Related issues:**
- Race condition between subscription and data emission
- Environment-dependent timing
- No synchronization mechanism
### Approach 2: Thread.sleep() (Workaround)
```java
publisher.onSubscribe(fakeSubscription);
new Thread(() -> {
try {
Thread.sleep(50); // Wait for subscription to complete
publisher.onNext("item1");
publisher.onNext("item2");
publisher.onCompleted();
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
publisher.onError(e);
}
}).start();
```
**Characteristics:**
- Reduces race condition probability but doesn't eliminate it
- Still relies on timing assumptions
- Minimal code changes
- Not guaranteed to work in all environments
### Approach 3: AssertSubscriber with CountDownLatch (Recommended)
```java
CountDownLatch subscribed = new CountDownLatch(1);
// In the mock setup
new Thread(() -> {
try {
if (subscribed.await(2, TimeUnit.SECONDS)) {
publisher.onNext("item1");
publisher.onNext("item2");
publisher.onCompleted();
} else {
publisher.onError(new IllegalStateException("Downstream not subscribed"));
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
publisher.onError(e);
}
}).start();
// In the test execution
AssertSubscriber<String> sub = AssertSubscriber.create(Long.MAX_VALUE);
multiResponse.subscribe().withSubscriber(sub);
sub.awaitSubscription(); // Ensure subscription is complete
subscribed.countDown(); // Signal that data can be emitted
```
**Advantages:**
- [x] Completely eliminates race condition
- [x] Environment-independent
- [x] Uses Mutiny's native testing tools
- [x] Clear synchronization semantics
- [x] Timeout protection
## Final Solution Implementation
Here's the complete fixed test method:
```java
@Test
void testOneToManyReturnsMultiAndEmitsItems() {
Invoker<Object> invoker = Mockito.mock(Invoker.class);
StubMethodDescriptor method = Mockito.mock(StubMethodDescriptor.class);
try (MockedStatic<StubInvocationUtil> mocked = Mockito.mockStatic(StubInvocationUtil.class)) {
AtomicBoolean stubCalled = new AtomicBoolean(false);
CountDownLatch subscribed = new CountDownLatch(1);
mocked.when(() -> StubInvocationUtil.serverStreamCall(
Mockito.eq(invoker), Mockito.eq(method), Mockito.eq("testRequest"), Mockito.any()))
.thenAnswer(invocation -> {
stubCalled.set(true);
ClientTripleMutinyPublisher<String> publisher = invocation.getArgument(3);
CallStreamObserver<String> fakeSubscription = new CallStreamObserver<>() {
@Override
public void request(int n) {
/* no-op */
}
@Override
public void setCompression(String compression) {}
@Override
public void disableAutoFlowControl() {}
@Override
public void onNext(String v) {
publisher.onNext(v);
}
@Override
public void onError(Throwable t) {
publisher.onError(t);
}
@Override
public void onCompleted() {
publisher.onCompleted();
}
};
publisher.onSubscribe(fakeSubscription);
// Wait for downstream subscription to complete before emitting data
new Thread(() -> {
try {
if (subscribed.await(5, TimeUnit.SECONDS)) {
publisher.onNext("item1");
publisher.onNext("item2");
publisher.onCompleted();
} else {
publisher.onError(
new IllegalStateException("Downstream subscription timeout"));
}
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
publisher.onError(e);
}
})
.start();
return null;
});
Uni<String> uniRequest = Uni.createFrom().item("testRequest");
Multi<String> multiResponse = MutinyClientCalls.oneToMany(invoker, uniRequest, method);
// Use AssertSubscriber to ensure proper subscription timing
AssertSubscriber<String> subscriber = AssertSubscriber.create(Long.MAX_VALUE);
multiResponse.subscribe().withSubscriber(subscriber);
// Wait for subscription to be established
subscriber.awaitSubscription();
subscribed.countDown(); // Signal that data emission can begin
// Wait for completion
subscriber.awaitCompletion(Duration.ofSeconds(5));
// Verify results
Assertions.assertTrue(stubCalled.get(), "StubInvocationUtil.serverStreamCall should be called");
Assertions.assertEquals(List.of("item1", "item2"), subscriber.getItems());
subscriber.assertCompleted();
}
}
```
## Key Benefits of This Solution
1. **Deterministic Execution**: Guarantees that data emission only occurs after subscription is complete
2. **Framework Integration**: Uses Mutiny's `AssertSubscriber` for proper reactive stream testing
3. **Environment Agnostic**: Works consistently across different environments and system loads
4. **Timeout Protection**: Includes safeguards against indefinite blocking
5. **Clear Intent**: Makes the synchronization requirements explicit and maintainable
This solution addresses the fundamental race condition while maintaining the test's original intent and providing robust execution guarantees across all environments.
## Checklist
- [x] Make sure there is a [GitHub_issue](https://github.com/apache/dubbo/issues) field for the change.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Write necessary unit-test to verify your logic correction. If the new feature or significant change is committed, please remember to add sample in [dubbo samples](https://github.com/apache/dubbo-samples) project.
- [x] Make sure gitHub actions can pass. [Why the workflow is failing and how to fix it?](../CONTRIBUTING.md)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix mutiny race condition issue [dubbo]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #15662:
URL: https://github.com/apache/dubbo/pull/15662#discussion_r2313977672
##########
dubbo-plugin/dubbo-mutiny/src/test/java/org/apache/dubbo/mutiny/MutinyClientCallsTest.java:
##########
@@ -100,7 +103,9 @@ void testOneToManyReturnsMultiAndEmitsItems() {
CallStreamObserver<String> fakeSubscription = new CallStreamObserver<>() {
@Override
- public void request(int n) {}
+ public void request(int n) {
+ /* no-op */
Review Comment:
[nitpick] The comment '/* no-op */' should be more descriptive to explain why this method is intentionally empty in the test context.
```suggestion
// Intentionally left empty: flow control is not required for this test's mock observer.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix mutiny race condition issue [dubbo]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #15662:
URL: https://github.com/apache/dubbo/pull/15662#issuecomment-3241966898
## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/15662?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 30.31%. Comparing base ([`5b9adb0`](https://app.codecov.io/gh/apache/dubbo/commit/5b9adb04df62b0ae800025187319b180a18cebab?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`4615517`](https://app.codecov.io/gh/apache/dubbo/commit/4615517639c0cf212baed48df57a313507fcb571?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
> :exclamation: There is a different number of reports uploaded between BASE (5b9adb0) and HEAD (4615517). Click for more details.
>
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (5b9adb0) | HEAD (4615517) |
>|------|------|------|
>|samples-tests-java21|1|0|
>|integration-tests-java21|1|0|
>|integration-tests-java8|1|0|
>|unit-tests-java8|1|0|
>|unit-tests-java17|1|0|
>|unit-tests-java11|1|0|
>|unit-tests-java21|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.3 #15662 +/- ##
=============================================
- Coverage 61.06% 30.31% -30.75%
+ Complexity 11704 9361 -2343
=============================================
Files 1923 1923
Lines 87069 87053 -16
Branches 13112 13028 -84
=============================================
- Hits 53165 26391 -26774
- Misses 28465 56587 +28122
+ Partials 5439 4075 -1364
```
| [Flag](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [integration-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [samples-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [samples-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `30.31% <ø> (-0.05%)` | :arrow_down: |
| [unit-tests-java11](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java17](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java21](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/15662/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/dubbo/pull/15662?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
Re: [PR] fix mutiny race condition issue [dubbo]
Posted by "oxsean (via GitHub)" <gi...@apache.org>.
oxsean merged PR #15662:
URL: https://github.com/apache/dubbo/pull/15662
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org