hnpkso opened a new issue, #16392: URL: https://github.com/apache/dubbo/issues/16392
### 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 fe239356d3 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 `fbac9ef284` — 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.mei 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, timezone-aligned) Nacos client log runs in a different timezone; both refer to the same wall clock. | Wall clock | Source | Event | |---|---|---| | T+0.264s | Nacos client | `init new ips(4)` — snapshot returned to `selectInstances`, contains the ghost | | T+0.237s | Dubbo | `Trying to subscribe from apps [3 apps]` — `subscribeURLs` enters | | T+0.282s | Dubbo | `Received instance notification, instances: 4` — pull result written into `allInstances` | | **T+0.356s** | **Nacos client** | **`removed ips(1)` — server pushes removal, absorbed by SDK cache** | | T+1.081s | Dubbo | `Start NettyClient /...20880` — synchronous metadata fetch begins | | T+1.582s | Dubbo | metadata fetch completes (500 ms) | | T+1.6xxs | Dubbo | `addServiceInstancesChangedListener` finally called — too late | The removal push arrives 92 ms after `subscribeURLs` starts and 1.3 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<String> serviceNames) { ... if (serviceInstancesChangedListener == null) { serviceInstancesChangedListener = serviceDiscovery.createListener(serviceNames); for (String serviceName : serviceNames) { List<ServiceInstance> 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. Frequency / recovery behavior Once the loss window has passed, allInstances stays stale until either: - another push event happens for the same appName (a later scale/deploy triggers reconciliation), or - the consumer process restarts. Neither is guaranteed. In our incident, the ghost survived for the entire process lifetime. 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 A PR is coming and will be linked here. 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) precisely so reviewers can git checkout each and observe the red → green transition 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) -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
