[Bug] Instance push loss during initial subscribe in ServiceDiscoveryRegistry
- Dominant language
- Java
- Stars
- 41.6k
- Forks
- 26.4k
- Avg merge
- 15h 13m
- Merged PRs (30d)
- 4
Description
### Pre-check
- [x] I am sure that all the content I provide is in English.
### Search before asking
- [x] I had searched in the [issues](https://github.com/apache/dubbo/issues?q=is%3Aissue) and found no similar issues.
### Apache Dubbo Component
Java SDK (apache/dubbo)
### Dubbo Version
- Dubbo: **3.2.19** in production, reproduced against **apache/3.3** HEAD
- Registry: **Nacos 2.x** (gRPC client, application-level discovery)
- Discovery mode: `APPLICATION_FIRST`
### Steps to reproduce this issue
The bug is deterministic and can be reproduced without a Nacos server, using only a failing unit test that captures the race.
1. Clone apache/dubbo at the 3.3 branch:
git clone https://github.com/apache/dubbo.git
cd dubbo
git checkout 3.3
2. Add my fork as a remote and fetch the branch:
git remote add hnpkso https://github.com/hnpkso/dubbo.git
git fetch hnpkso fix/subscribe-push-loss
3. Check out just the test commit (source stays as unmodified 3.3):
git checkout f9755db2
4. Run the tests:
mvn -pl dubbo-registry/dubbo-registry-api test \
-Dtest=ServiceDiscoveryRegistryTest
5. Observe 3 failures out of 6:
- testSubscribeURLsRegistersPushCallbackBeforePull
InOrder verify: addServiceInstancesChangedListener must precede
getInstances. Fails today because it runs afterwards.
- testSubscribeURLsSkipsPullWhenPushAlreadyPopulated
A mocked push populates allInstances the moment the callback is
registered; the code proceeds to pull anyway and would overwrite
the fresher push snapshot.
- testSubscribeURLs
A pre-existing test's `verify(..., times(2))` on the register call
captures a related idempotency issue: register is invoked on every
subscribeURLs() call, not once per listener lifetime.
6. Check out the fix commit `8e0f8f2d81` — all 6 tests pass.
The failing tests directly model the production race:
ServiceDiscoveryRegistry.subscribeURLs() calls getInstances() and runs
the resulting onEvent() (which synchronously fetches metadata over the
network) BEFORE registering the push callback via
addServiceInstancesChangedListener(). Any push arriving in this window
is absorbed by the discovery SDK's local cache and never reaches Dubbo.
### What you expected to happen
### Expected
When Nacos removes a provider instance during the consumer's initial subscribe, the removal push should be delivered to Dubbo's ServiceInstancesChangedListener and update `allInstances`. The consumer's
invoker table should reflect the current state of the registry.
### What actually happened
`ServiceDiscoveryRegistry.subscribeURLs()` performs its initial `getInstances()` pull before registering the push callback via `serviceDiscovery.addServiceInstancesChangedListener(...)`.
The pull path also runs `ServiceInstancesChangedListener.onEvent()`, which fetches metadata over the network — in production this took ~500 ms. Any push arriving between the pull and the register call is
absorbed by the discovery SDK's local cache and never delivered to Dubbo. From that point on, `allInstances` holds a permanently stale snapshot until either:
- the same appName has a later, independent push event (which then reconciles), or
- the consumer restarts.
Neither is guaranteed to happen; we saw a stale entry survive for the entire process lifetime.
### Impact — production incident
Consumer routes RPCs to a provider that no longer exports the service, producing recurring errors:
```
RpcException: Failed to invoke the method queryList in the service ...
provider: DefaultServiceInstance{host='10.87.175.50', port=20880, ...}
cause: RemotingException: Fail to decode request due to:
RpcInvocation [methodName=queryList, parameterTypes=null]
```
The address `10.87.175.50` had already been removed from Nacos before the consumer's subscribe completed, but the removal push landed inside the loss window.
### Timeline (production log)
The Nacos client log and Dubbo log run in different timezones; both refer to the same wall clock. Offsets below are relative to when `subscribeURLs` entered.
| Offset | Source | Event |
|---|---|---|
| T+0ms | Dubbo | `Trying to subscribe from apps [3 apps]` — `subscribeURLs` enters |
| T+27ms | Nacos client | `init new ips(4)` — snapshot returned to `selectInstances`, contains the ghost |
| T+45ms | Dubbo | `Received instance notification, instances: 4` — pull result written into `allInstances` |
| **T+119ms** | **Nacos client** | **`removed ips(1)` — server pushes removal, absorbed by SDK cache** |
| T+844ms | Dubbo | `Start NettyClient /...:20880` — synchronous metadata fetch begins |
| T+1345ms | Dubbo | metadata fetch completes (~500 ms elapsed) |
| T+1.4s+ | Dubbo | `addServiceInstancesChangedListener` finally called — too late |
The removal push arrives 119 ms after `subscribeURLs` starts and ~1.2 s before the push callback is registered.
### Anything else
### Root cause (code reference on apache/dubbo `3.3`)
`dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistry.java`:
```java
protected void subscribeURLs(URL url, NotifyListener listener, Set serviceNames) {
...
if (serviceInstancesChangedListener == null) {
serviceInstancesChangedListener = serviceDiscovery.createListener(serviceNames);
for (String serviceName : serviceNames) {
List serviceInstances =
serviceDiscovery.getInstances(serviceName); // (1) pull
if (CollectionUtils.isNotEmpty(serviceInstances)) {
serviceInstancesChangedListener.onEvent(...); // (2) blocks on metadata fetch
}
}
serviceListeners.put(serviceNamesKey, serviceInstancesChangedListener);
}
...
serviceDiscovery.addServiceInstancesChangedListener(...); // (3) register — too late
}
```
Between (1) and (3), the discovery SDK is holding pushes without a subscriber attached to Dubbo. Those pushes never reach `ServiceInstancesChangedListener.doOnEvent()`, so `allInstances` is never refreshed.
### Affected versions
Same code path exists unchanged on 3.2, 3.3, and 3.4 — the last touch to this method predates the fix window. The `MappingListener` sibling race in the same method was fixed by #14851, but the instance-list side (this issue) was left untouched.
### Proposed fix
PR #16393 is open. Summary:
- register `addServiceInstancesChangedListener` before the initial pull;
- serialize the pull loop with `synchronized(listener)` so it shares the intrinsic lock that `doOnEvent` already holds;
- skip the pull for any `serviceName` a push has already populated (push wins over stale pull).
The PR ships two commits (test + fix) so reviewers can `git checkout` each and observe the tests transition from failing to passing, without any Nacos setup.
### Do you have a (mini) reproduction demo?
- [x] Yes, I have a minimal reproduction demo to help resolve this issue more effectively!
### Are you willing to submit a pull request to fix on your own?
- [x] Yes I am willing to submit a pull request on my own!
### Code of Conduct
- [x] I agree to follow this project's [Code of Conduct](https://www.apache.org/foundation/policies/conduct)
Contributor guide
Research direction
Start with dubbo-registry/dubbo-registry-api/src/main/java/org/apache/dubbo/registry/client/ServiceDiscoveryRegistry.java and the ServiceDiscoveryRegistryTest tests described in the issue. Run the Maven test command against the 3.3 branch and compare the failing test commit with PR #16393. Done means the callback-registration race, push-over-pull behavior, and listener idempotency tests pass.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- java
- Domain
- backend, distributed-systems
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Stale
- Clarity
- Clearly specified
- Newbie friendliness
- 25/100