hnpkso opened a new pull request, #16393: URL: https://github.com/apache/dubbo/pull/16393
# PR: Fix instance push loss during initial subscribe in ServiceDiscoveryRegistry **Base branch**: `apache/dubbo:3.3` (also applies cleanly to `3.2` and `3.4`) **Commits**: 2 (test-only `fe239356d3`, then fix `fbac9ef284`). Cherry-pick either to A/B verify. --- ## What this fixes Fixes #16392. `ServiceDiscoveryRegistry.subscribeURLs()` currently: 1. calls `serviceDiscovery.getInstances(serviceName)` — the initial pull, 2. runs `listener.onEvent(...)` which synchronously fetches metadata (can take seconds), 3. **only then** calls `serviceDiscovery.addServiceInstancesChangedListener(...)` to arm the push callback. Any push arriving between step 1 and step 3 is absorbed by the discovery SDK's local cache and never reaches Dubbo. `allInstances` is then permanently stale until either (a) a later, independent push happens for the same appName, or (b) the process restarts. We hit this in production (Nacos 2.x, Dubbo 3.2.19): a provider Nacos had removed 92 ms after our subscribe started remained a ghost invoker for the entire process lifetime, producing recurring `Fail to decode request` errors. Timeline and full analysis are in the linked issue. ## The change `dubbo-registry-api/…/ServiceDiscoveryRegistry.java` — `subscribeURLs()`: 1. **Register the push callback before the initial pull.** `addServiceInstancesChangedListener(...)` moves inside the `if (listener == null)` guard, ahead of the pull loop. Any push arriving during/after the pull now flows through the listener's already-armed `doOnEvent` path. 2. **Serialize the pull with the push callback.** Wrap the pull loop in `synchronized(serviceInstancesChangedListener)`. `doOnEvent` is a `synchronized` method on the same object; both now share the same intrinsic lock, so a push arriving during initialization queues cleanly instead of interleaving writes to `allInstances`. 3. **Push wins over stale pull.** Inside the synchronized region, check `listener.getAllInstances().containsKey(serviceName)` before pulling. If a push has already populated this service, treat push as authoritative and skip the pull. This prevents a slow pull (reading a stale SDK cache) from overwriting a fresher push snapshot. Diff is under 30 lines. ## A/B verification (30 seconds for reviewer) Two commits are separated intentionally: ```bash git remote add hnpkso https://github.com/hnpkso/dubbo.git git fetch hnpkso fix/subscribe-push-loss git checkout fe239356d3 # test commit only mvn -pl dubbo-registry/dubbo-registry-api test \ -Dtest=ServiceDiscoveryRegistryTest # Tests run: 6, Failures: 3 # ✗ testSubscribeURLs (times(2) assertion on register) # ✗ testSubscribeURLsRegistersPushCallbackBeforePull ← MAIN # ✗ testSubscribeURLsSkipsPullWhenPushAlreadyPopulated git checkout fbac9ef284 # fix on top mvn -pl dubbo-registry/dubbo-registry-api test \ -Dtest=ServiceDiscoveryRegistryTest # Tests run: 6, Failures: 0 ``` The red-to-green transition **is** the reproduction — no Nacos or external system needed. Full module test suite: `Tests run: 111, Failures: 0`. ## Behavior changes reviewer should validate 1. **`addServiceInstancesChangedListener` cardinality**: now called once per listener lifetime, previously once per `subscribeURLs()` call. All in-tree `ServiceDiscovery` implementations already treat duplicate calls as no-ops via `instanceListeners.add()` early-return, so the change is observationally equivalent. Test assertion in `testSubscribeURLs` updates from `times(2)` to `times(1)`. 2. **`RegistryEvent.toSsEvent` metric cardinality** rides along with (1): it now fires once per listener creation instead of once per subscribe. If per-subscribe metric cardinality must be preserved, `MetricsEventBus.post(...)` can be split back out of the register lambda without affecting correctness. Happy to change on request. 3. **Push executor threads may briefly park** on the listener intrinsic lock during initial subscribe, bounded by the metadata fetch duration (~500 ms in our production). Previously such pushes were silently dropped. Strict improvement, but worth noting. ## Scope, and what is intentionally not in this PR - **Not touched: `NacosServiceDiscovery` or other `ServiceDiscovery` implementations.** The fix lives at the registry orchestration layer, so implementations are unchanged and no new SPI contract is introduced. - **Not addressed: register-throws-then-orphan-listener.** If `addServiceInstancesChangedListener` throws after `serviceListeners.put(...)`, an unlinked listener is left cached. This exists in the current code too (in a slightly different form) and requires broader try/rollback plumbing — worth a follow-up PR. - **Multi-registry (`MultipleServiceDiscovery`)**: delegates per-child, behavior should match, but I did not extend testing there. ## Related - #14851 fixed a sibling race on the `MappingListener` side of the same subscribe path (initial-apps was `null` when the listener was constructed). The instance-list side, addressed here, has the same shape of bug (populate-before-arm) but a different failure mode (silent instance loss vs. spurious mapping event). ## Commits - `fe239356d3` — `Add failing tests for instance push loss during initial subscribe`. Tests only, fails on unmodified 3.3. - `fbac9ef284` — `Fix instance push loss during initial subscribe in ServiceDiscoveryRegistry`. Source fix, turns tests green. Feel free to squash on merge; the two commits exist purely to make the red→green transition observable during review. -- 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]
