Repository: go-kratos/kratos
Pull request #2647 (2023-02-03 03:48:44)

fix: do not re register after service logout
<!--
🎉 Thanks for sending a pull request to Kratos! Here are some tips for you:

1. If this is your first time contributing to Kratos, please read our contribution guide: https://go-kratos.dev/en/docs/community/contribution/
2. Ensure you have added or ran the appropriate tests and lint for your PR, please use `make lint` and `make test` before filing your PR, use `make clean` to tidy your go mod.
3. If the PR is unfinished, you may need to mark it as a WIP(Work In Progress) PR or draft PR
4. Please use a semantic commits format title, such as `<type>[optional scope]: <description>`, see: https://go-kratos.dev/docs/community/contribution#type
5. at the same time, please note that similar work should be submitted in one PR as far as possible to reduce the workload of reviewers. Do not split a work into multiple PR unless it should.
-->

<!--
🎉 感谢您向 Kratos 发送 PR 请求！以下是一些提示：
如果这是你第一次为 Kratos 贡献，请阅读我们的贡献指南：https://go-kratos.dev/en/docs/community/contribution/
2、确保您已经为您的 PR 添加或运行了适当的测试和lint，请在提交PR之前使用“make lint”和“make test”，使用“make clean”整理您的 go.mod。
3、如果请购单未完成，您可能需要将其标记为 WIP（在制品）PR 或 draft PR
4、请使用语义提交格式标题，如“<类型>[可选范围]：<说明>`，请参阅：https://go-kratos.dev/docs/community/contribution#type
5. 同时请注意，同类的工作请尽量在一个PR中提交，以减轻 review 者的工作负担，不要把一项工作拆分成很多个PR，除非它应该这样做。
-->

<!--
-->

#### Description (what this PR does / why we need it):
<!--
* The description should include the motivation for this PR or contrast this with previous behavior
-->


#### Which issue(s) this PR fixes (resolves / be part of):
<!--
* Automatically closes linked issue when PR is merged.
* If your PR is not fully resolved the issue, please use `part of #<issue number>` instead.

Usage: `fixes/resolves #<issue number>`, or `fixes/resolves (paste link of issue)`.
-->


#### Other special notes for the reviewers:
<!--
* Somethings that need extra attention for the reviewers
* Some additional notes, TODO list, etc.
-->


codecov-commenter: # [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/2647?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) Report
> Merging [#2647](https://codecov.io/gh/go-kratos/kratos/pull/2647?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (5ba5aa8) into [main](https://codecov.io/gh/go-kratos/kratos/commit/239121155d0f74c56645ae0a0d729789e12b1682?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (2391211) will **decrease** coverage by `0.06%`.
> The diff coverage is `n/a`.

:mega: This organization is not using Codecov’s [GitHub App Integration](https://github.com/apps/codecov). We recommend you install it so Codecov can continue to function properly for your repositories. [Learn more](https://about.codecov.io/blog/codecov-is-updating-its-github-integration/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)

```diff
@@            Coverage Diff             @@
##             main    #2647      +/-   ##
==========================================
- Coverage   84.87%   84.82%   -0.06%     
==========================================
  Files          88       88              
  Lines        3875     3875              
==========================================
- Hits         3289     3287       -2     
- Misses        411      413       +2     
  Partials      175      175              
```


| [Impacted Files](https://codecov.io/gh/go-kratos/kratos/pull/2647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) | Coverage Δ | |
|---|---|---|
| [internal/context/context.go](https://codecov.io/gh/go-kratos/kratos/pull/2647?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-aW50ZXJuYWwvY29udGV4dC9jb250ZXh0Lmdv) | `97.01% <0.00%> (-2.99%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)


Patch:
diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml
index 3a6bcb31..9d94238a 100644
--- a/.github/workflows/go.yml
+++ b/.github/workflows/go.yml
@@ -13,7 +13,7 @@ jobs:
   build:
     strategy:
       matrix:
-        go: [1.18,1.19]
+        go: [1.18,1.19,1.20.x]
     name: build & test
     runs-on: ubuntu-latest
     services:
@@ -82,4 +82,4 @@ jobs:
         run: |
           cd cmd/protoc-gen-go-http
           go build ./...
-          go test ./...
\ No newline at end of file
+          go test ./...
diff --git a/contrib/registry/consul/client.go b/contrib/registry/consul/client.go
index 31d0da68..ea5fe846 100644
--- a/contrib/registry/consul/client.go
+++ b/contrib/registry/consul/client.go
@@ -2,6 +2,7 @@ package consul
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"math/rand"
 	"net"
@@ -204,21 +205,32 @@ func (c *Client) Register(_ context.Context, svc *registry.ServiceInstance, enab
 			defer ticker.Stop()
 			for {
 				select {
+				case <-c.ctx.Done():
+					_ = c.cli.Agent().ServiceDeregister(svc.ID)
+					return
+				default:
+				}
+				select {
+				case <-c.ctx.Done():
+					_ = c.cli.Agent().ServiceDeregister(svc.ID)
+					return
 				case <-ticker.C:
+					// ensure that unregistered services will not be re-registered by mistake
+					if errors.Is(c.ctx.Err(), context.Canceled) || errors.Is(c.ctx.Err(), context.DeadlineExceeded) {
+						_ = c.cli.Agent().ServiceDeregister(svc.ID)
+						return
+					}
 					err = c.cli.Agent().UpdateTTL("service:"+svc.ID, "pass", "pass")
 					if err != nil {
 						log.Errorf("[Consul] update ttl heartbeat to consul failed! err=%v", err)
 						// when the previous report fails, try to re register the service
-						time.AfterFunc(time.Duration(rand.Intn(5))*time.Second, func() {
-							if err := c.cli.Agent().ServiceRegister(asr); err != nil {
-								log.Errorf("[Consul] re registry service failed!, err=%v", err)
-							} else {
-								log.Warn("[Consul] re registry of service occurred success")
-							}
-						})
+						time.Sleep(time.Duration(rand.Intn(5)) * time.Second)
+						if err := c.cli.Agent().ServiceRegister(asr); err != nil {
+							log.Errorf("[Consul] re registry service failed!, err=%v", err)
+						} else {
+							log.Warn("[Consul] re registry of service occurred success")
+						}
 					}
-				case <-c.ctx.Done():
-					return
 				}
 			}
 		}()
@@ -228,6 +240,6 @@ func (c *Client) Register(_ context.Context, svc *registry.ServiceInstance, enab
 
 // Deregister service by service ID
 func (c *Client) Deregister(_ context.Context, serviceID string) error {
-	c.cancel()
+	defer c.cancel()
 	return c.cli.Agent().ServiceDeregister(serviceID)
 }
diff --git a/middleware/recovery/recovery.go b/middleware/recovery/recovery.go
index dfa7cb7f..66dcec18 100644
--- a/middleware/recovery/recovery.go
+++ b/middleware/recovery/recovery.go
@@ -29,12 +29,6 @@ func WithHandler(h HandlerFunc) Option {
 	}
 }
 
-// WithLogger with recovery logger.
-// Deprecated: use global logger instead.
-func WithLogger(logger log.Logger) Option {
-	return func(o *options) {}
-}
-
 // Recovery is a server middleware that recovers from any panics.
 func Recovery(opts ...Option) middleware.Middleware {
 	op := options{


Test patch:
diff --git a/contrib/polaris/registry_test.go b/contrib/polaris/registry_test.go
index 7295de53..a03f807e 100644
--- a/contrib/polaris/registry_test.go
+++ b/contrib/polaris/registry_test.go
@@ -29,8 +29,8 @@ func TestRegistry(t *testing.T) {
 	)
 
 	ins := &registry.ServiceInstance{
-		ID:      "test-ut-1",
-		Name:    "test-ut-1",
+		ID:      "test-ut",
+		Name:    "test-ut",
 		Version: "v1.0.0",
 		Endpoints: []string{
 			"grpc://127.0.0.1:8080",
@@ -50,115 +50,9 @@ func TestRegistry(t *testing.T) {
 		t.Fatal(err)
 	}
 	time.Sleep(time.Second * 3)
-	service, err := r.GetService(context.Background(), "test-ut-1")
+	service, err := r.GetService(context.Background(), "test-ut")
 	if err != nil {
 		t.Fatal(err)
 	}
 	t.Log(service)
 }
-
-func TestWatch(t *testing.T) {
-	sdk, err := polaris.NewSDKContextByAddress("127.0.0.1:8091")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	p := New(sdk)
-
-	r := p.Registry(
-		WithRegistryTimeout(time.Second),
-		WithRegistryHealthy(true),
-		WithRegistryIsolate(false),
-		WithRegistryRetryCount(3),
-		WithRegistryWeight(100),
-		WithRegistryTTL(1000),
-	)
-
-	err = r.Register(context.Background(), &registry.ServiceInstance{
-		ID:      "test-ut",
-		Name:    "test-ut",
-		Version: "v1.0.0",
-		Endpoints: []string{
-			"grpc://127.0.0.1:8080",
-			"http://127.0.0.1:9090",
-		},
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	time.Sleep(time.Second * 3)
-	w, err := r.Watch(context.Background(), "test-ut")
-	if err != nil {
-		t.Fatal(err)
-	}
-	service, err := w.Next()
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	if len(service) != 1 {
-		t.Fatalf("want 1, got %d, service %+v", len(service), service)
-	}
-
-	err = r.Register(context.Background(), &registry.ServiceInstance{
-		ID:      "test-ut2",
-		Name:    "test-ut",
-		Version: "v1.0.0",
-		Endpoints: []string{
-			"grpc://127.0.0.2:8080",
-			"http://127.0.0.2:9090",
-		},
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	time.Sleep(time.Second * 3)
-	service, err = w.Next()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(service) != 2 {
-		t.Fatalf("want 1, got %d, service %+v", len(service), service)
-	}
-
-	err = r.Deregister(context.Background(), &registry.ServiceInstance{
-		ID:      "test-ut",
-		Name:    "test-ut",
-		Version: "v1.0.0",
-		Endpoints: []string{
-			"grpc://127.0.0.1:8080",
-			"http://127.0.0.1:9090",
-		},
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	time.Sleep(time.Second * 3)
-	service, err = w.Next()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(service) != 1 {
-		t.Fatalf("want 1, got %d, service %+v", len(service), service)
-	}
-	err = r.Deregister(context.Background(), &registry.ServiceInstance{
-		ID:      "test-ut",
-		Name:    "test-ut",
-		Version: "v1.0.0",
-		Endpoints: []string{
-			"grpc://127.0.0.2:8080",
-			"http://127.0.0.2:9090",
-		},
-	})
-	if err != nil {
-		t.Fatal(err)
-	}
-	time.Sleep(time.Second * 3)
-	service, err = w.Next()
-	if err != nil {
-		t.Fatal(err)
-	}
-	if len(service) != 0 {
-		t.Fatalf("want 0, got %d", len(service))
-	}
-}
diff --git a/contrib/registry/consul/registry_test.go b/contrib/registry/consul/registry_test.go
index a6025123..791c2efc 100644
--- a/contrib/registry/consul/registry_test.go
+++ b/contrib/registry/consul/registry_test.go
@@ -74,14 +74,14 @@ func TestRegistry_Register(t *testing.T) {
 				serverName: "server-1",
 				server: []*registry.ServiceInstance{
 					{
-						ID:        "1",
+						ID:        "2",
 						Name:      "server-1",
 						Version:   "v0.0.1",
 						Metadata:  nil,
 						Endpoints: []string{"http://127.0.0.1:8000"},
 					},
 					{
-						ID:        "1",
+						ID:        "2",
 						Name:      "server-1",
 						Version:   "v0.0.2",
 						Metadata:  nil,
@@ -91,7 +91,7 @@ func TestRegistry_Register(t *testing.T) {
 			},
 			want: []*registry.ServiceInstance{
 				{
-					ID:        "1",
+					ID:        "2",
 					Name:      "server-1",
 					Version:   "v0.0.2",
 					Metadata:  nil,
@@ -168,6 +168,13 @@ func TestRegistry_GetService(t *testing.T) {
 		Endpoints: []string{fmt.Sprintf("tcp://%s?isSecure=false", addr)},
 	}
 
+	instance2 := &registry.ServiceInstance{
+		ID:        "2",
+		Name:      "server-1",
+		Version:   "v0.0.1",
+		Endpoints: []string{fmt.Sprintf("tcp://%s?isSecure=false", addr)},
+	}
+
 	type fields struct {
 		registry *Registry
 	}
@@ -223,10 +230,10 @@ func TestRegistry_GetService(t *testing.T) {
 			want:    nil,
 			wantErr: true,
 			preFunc: func(t *testing.T) {
-				if err := r.Register(context.Background(), instance1); err != nil {
+				if err := r.Register(context.Background(), instance2); err != nil {
 					t.Error(err)
 				}
-				watch, err := r.Watch(context.Background(), instance1.Name)
+				watch, err := r.Watch(context.Background(), instance2.Name)
 				if err != nil {
 					t.Error(err)
 				}
@@ -236,7 +243,7 @@ func TestRegistry_GetService(t *testing.T) {
 				}
 			},
 			deferFunc: func(t *testing.T) {
-				err := r.Deregister(context.Background(), instance1)
+				err := r.Deregister(context.Background(), instance2)
 				if err != nil {
 					t.Error(err)
 				}
@@ -282,6 +289,20 @@ func TestRegistry_Watch(t *testing.T) {
 		Endpoints: []string{fmt.Sprintf("tcp://%s?isSecure=false", addr)},
 	}
 
+	instance2 := &registry.ServiceInstance{
+		ID:        "2",
+		Name:      "server-1",
+		Version:   "v0.0.1",
+		Endpoints: []string{fmt.Sprintf("tcp://%s?isSecure=false", addr)},
+	}
+
+	instance3 := &registry.ServiceInstance{
+		ID:        "3",
+		Name:      "server-1",
+		Version:   "v0.0.1",
+		Endpoints: []string{fmt.Sprintf("tcp://%s?isSecure=false", addr)},
+	}
+
 	type args struct {
 		ctx      context.Context
 		cancel   func()
@@ -316,7 +337,7 @@ func TestRegistry_Watch(t *testing.T) {
 			args: args{
 				ctx:      canceledCtx,
 				cancel:   cancel,
-				instance: instance1,
+				instance: instance2,
 				opts: []Option{
 					WithHealthCheck(false),
 				},
@@ -330,14 +351,14 @@ func TestRegistry_Watch(t *testing.T) {
 			name: "register with healthCheck",
 			args: args{
 				ctx:      context.Background(),
-				instance: instance1,
+				instance: instance3,
 				opts: []Option{
 					WithHeartbeat(true),
 					WithHealthCheck(true),
 					WithHealthCheckInterval(5),
 				},
 			},
-			want:    []*registry.ServiceInstance{instance1},
+			want:    []*registry.ServiceInstance{instance3},
 			wantErr: false,
 			preFunc: func(t *testing.T) {
 				lis, err := net.Listen("tcp", addr)
diff --git a/middleware/recovery/recovery_test.go b/middleware/recovery/recovery_test.go
index 985cb19b..cf4a7fbb 100644
--- a/middleware/recovery/recovery_test.go
+++ b/middleware/recovery/recovery_test.go
@@ -6,7 +6,6 @@ import (
 	"testing"
 
 	"github.com/go-kratos/kratos/v2/errors"
-	"github.com/go-kratos/kratos/v2/log"
 )
 
 func TestOnce(t *testing.T) {
@@ -35,8 +34,3 @@ func TestNotPanic(t *testing.T) {
 		t.Errorf("e isn't nil")
 	}
 }
-
-// Deprecated: Remove this test with WithLogger method.
-func TestWithLogger(t *testing.T) {
-	_ = WithLogger(log.DefaultLogger)
-}