You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by "hnpkso (via GitHub)" <gi...@apache.org> on 2026/07/23 04:58:43 UTC
[PR] Fix instance push loss during initial subscribe in ServiceDiscoveryRegistry [dubbo]
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: 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 instance push loss during initial subscribe in ServiceDiscoveryRegistry [dubbo]
Posted by "hnpkso (via GitHub)" <gi...@apache.org>.
hnpkso commented on PR #16393:
URL: https://github.com/apache/dubbo/pull/16393#issuecomment-5213771085
Thanks @zrlw for the review!
@EarthChen would appreciate a second look when you have a moment — production race fix for the initial-subscribe path against Nacos 2.x, deterministic tests included, one approval already in.
--
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 instance push loss during initial subscribe in ServiceDiscoveryRegistry [dubbo]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #16393:
URL: https://github.com/apache/dubbo/pull/16393#issuecomment-5054846240
## [Codecov](https://app.codecov.io/gh/apache/dubbo/pull/16393?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `85.71429%` with `2 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 32.18%. Comparing base ([`eb1d8ab`](https://app.codecov.io/gh/apache/dubbo/commit/eb1d8abaebdc2ce1e15d6236cf9f9179d34e9082?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`8e0f8f2`](https://app.codecov.io/gh/apache/dubbo/commit/8e0f8f2d811003a1253ea1297e7faac2ef3c4924?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing lines](https://app.codecov.io/gh/apache/dubbo/pull/16393?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ubbo/registry/client/ServiceDiscoveryRegistry.java](https://app.codecov.io/gh/apache/dubbo/pull/16393?src=pr&el=tree&filepath=dubbo-registry%2Fdubbo-registry-api%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fdubbo%2Fregistry%2Fclient%2FServiceDiscoveryRegistry.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZHViYm8tcmVnaXN0cnkvZHViYm8tcmVnaXN0cnktYXBpL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9kdWJiby9yZWdpc3RyeS9jbGllbnQvU2VydmljZURpc2NvdmVyeVJlZ2lzdHJ5LmphdmE=) | 85.71% | [0 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/dubbo/pull/16393?src=pr&el=tree&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 (eb1d8ab) and HEAD (8e0f8f2). Click for more details.
>
> <details><summary>HEAD has 7 uploads less than BASE</summary>
>
>| Flag | BASE (eb1d8ab) | HEAD (8e0f8f2) |
>|------|------|------|
>|integration-tests-java21|1|0|
>|integration-tests-java8|1|0|
>|unit-tests-java25|1|0|
>|unit-tests-java8|1|0|
>|unit-tests-java11|1|0|
>|unit-tests-java21|1|0|
>|unit-tests-java17|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 3.3 #16393 +/- ##
=============================================
- Coverage 60.90% 32.18% -28.73%
+ Complexity 11763 10231 -1532
=============================================
Files 1953 1952 -1
Lines 89266 89247 -19
Branches 13471 13388 -83
=============================================
- Hits 54367 28721 -25646
- Misses 29319 56318 +26999
+ Partials 5580 4208 -1372
```
| [Flag](https://app.codecov.io/gh/apache/dubbo/pull/16393/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/16393/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/16393/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/16393/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `32.11% <85.71%> (-0.05%)` | :arrow_down: |
| [samples-tests-java8](https://app.codecov.io/gh/apache/dubbo/pull/16393/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `29.78% <85.71%> (-0.04%)` | :arrow_down: |
| [unit-tests-java11](https://app.codecov.io/gh/apache/dubbo/pull/16393/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/16393/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/16393/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unit-tests-java25](https://app.codecov.io/gh/apache/dubbo/pull/16393/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/16393/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 Harness](https://app.codecov.io/gh/apache/dubbo/pull/16393?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 instance push loss during initial subscribe in ServiceDiscoveryRegistry [dubbo]
Posted by "EarthChen (via GitHub)" <gi...@apache.org>.
EarthChen merged PR #16393:
URL: https://github.com/apache/dubbo/pull/16393
--
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