Skip to content

Fix NAOCS close client bug - #369

Open
mlboy wants to merge 5 commits into
zeromicro:mainfrom
mlboy:main
Open

mlboy wants to merge 5 commits into
zeromicro:mainfrom
mlboy:main

Conversation

@mlboy

@mlboy mlboy commented Sep 8, 2026

Copy link
Copy Markdown

Greptile Summary

This PR corrects the Nacos resolver shutdown lifecycle:

  • Retains subscription parameters so the resolver can unsubscribe during shutdown.
  • Cancels endpoint processing, unsubscribes, closes the naming client, and drains pending callback data.
  • Buffers the callback channel to reduce shutdown-time blocking.
  • Restores the canonical Go module path following the previous review.

Confidence Score: 5/5

The PR appears safe to merge, with no outstanding correctness, security, or repository-rule issues identified.

The previously reported module-path mismatch is fully fixed by restoring the canonical github.com/zeromicro/zero-contrib/zrpc/registry/nacos declaration, and no new actionable issues were introduced by that correction.

Important Files Changed

Filename Overview
zrpc/registry/nacos/builder.go Builds a buffered callback pipeline and retains the Nacos client and subscription parameters for orderly shutdown.
zrpc/registry/nacos/resolver.go Makes resolver closure idempotently cancel processing, unsubscribe, close the client, and drain pending callback data.
zrpc/registry/nacos/resolver_test.go Updates the test client and watcher tests for the new unsubscribe-aware resolver lifecycle.

Sequence Diagram

sequenceDiagram
    participant Resolver
    participant Context
    participant Nacos
    participant Pipe
    Resolver->>Context: cancel()
    Resolver->>Nacos: Unsubscribe(subParam)
    Resolver->>Nacos: CloseClient()
    Resolver->>Pipe: drain pending callback data
Loading

Reviews (3): Last reviewed commit: "Fix nacos module path" | Re-trigger Greptile

Comment thread zrpc/registry/nacos/go.mod Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant