From 5bd84c24bc414ed8e33177a0c64d2e5993a358d4 Mon Sep 17 00:00:00 2001 From: Ryan Leung Date: Tue, 2 Jan 2024 14:51:31 +0800 Subject: [PATCH] api: fix panic when region doesn't have a leader (#7629) close tikv/pd#7630 Signed-off-by: Ryan Leung Co-authored-by: ti-chi-bot[bot] <108142056+ti-chi-bot[bot]@users.noreply.github.com> --- server/api/region.go | 66 ++++++++++++++++++------ tools/pd-ctl/tests/region/region_test.go | 53 +++++++++++++++++++ 2 files changed, 103 insertions(+), 16 deletions(-) diff --git a/server/api/region.go b/server/api/region.go index 62713cb6dcd..740a2c84dde 100644 --- a/server/api/region.go +++ b/server/api/region.go @@ -256,7 +256,13 @@ func (h *regionHandler) GetRegionByID(w http.ResponseWriter, r *http.Request) { } regionInfo := rc.GetRegion(regionID) - h.rd.JSON(w, http.StatusOK, NewAPIRegionInfo(regionInfo)) + b, err := marshalRegionInfoJSON(r.Context(), regionInfo) + if err != nil { + h.rd.JSON(w, http.StatusInternalServerError, err.Error()) + return + } + + h.rd.Data(w, http.StatusOK, b) } // @Tags region @@ -286,7 +292,13 @@ func (h *regionHandler) GetRegion(w http.ResponseWriter, r *http.Request) { } regionInfo := rc.GetRegionByKey([]byte(key)) - h.rd.JSON(w, http.StatusOK, NewAPIRegionInfo(regionInfo)) + b, err := marshalRegionInfoJSON(r.Context(), regionInfo) + if err != nil { + h.rd.JSON(w, http.StatusInternalServerError, err.Error()) + return + } + + h.rd.Data(w, http.StatusOK, b) } // @Tags region @@ -323,6 +335,24 @@ func newRegionsHandler(svr *server.Server, rd *render.Render) *regionsHandler { } } +// marshalRegionInfoJSON marshals region to bytes in `RegionInfo`'s JSON format. +// It is used to reduce the cost of JSON serialization. +func marshalRegionInfoJSON(ctx context.Context, r *core.RegionInfo) ([]byte, error) { + out := &jwriter.Writer{} + + region := &RegionInfo{} + select { + case <-ctx.Done(): + // Return early, avoid the unnecessary computation. + // See more details in https://github.com/tikv/pd/issues/6835 + return nil, ctx.Err() + default: + } + + covertAPIRegionInfo(r, region, out) + return out.Buffer.BuildBytes(), out.Error +} + // marshalRegionsInfoJSON marshals regions to bytes in `RegionsInfo`'s JSON format. // It is used to reduce the cost of JSON serialization. func marshalRegionsInfoJSON(ctx context.Context, regions []*core.RegionInfo) ([]byte, error) { @@ -346,20 +376,7 @@ func marshalRegionsInfoJSON(ctx context.Context, regions []*core.RegionInfo) ([] if i > 0 { out.RawByte(',') } - InitRegion(r, region) - // EasyJSON will not check anonymous struct pointer field and will panic if the field is nil. - // So we need to set the field to default value explicitly when the anonymous struct pointer is nil. - region.Leader.setDefaultIfNil() - for i := range region.Peers { - region.Peers[i].setDefaultIfNil() - } - for i := range region.PendingPeers { - region.PendingPeers[i].setDefaultIfNil() - } - for i := range region.DownPeers { - region.DownPeers[i].setDefaultIfNil() - } - region.MarshalEasyJSON(out) + covertAPIRegionInfo(r, region, out) } out.RawByte(']') @@ -367,6 +384,23 @@ func marshalRegionsInfoJSON(ctx context.Context, regions []*core.RegionInfo) ([] return out.Buffer.BuildBytes(), out.Error } +func covertAPIRegionInfo(r *core.RegionInfo, region *RegionInfo, out *jwriter.Writer) { + InitRegion(r, region) + // EasyJSON will not check anonymous struct pointer field and will panic if the field is nil. + // So we need to set the field to default value explicitly when the anonymous struct pointer is nil. + region.Leader.setDefaultIfNil() + for i := range region.Peers { + region.Peers[i].setDefaultIfNil() + } + for i := range region.PendingPeers { + region.PendingPeers[i].setDefaultIfNil() + } + for i := range region.DownPeers { + region.DownPeers[i].setDefaultIfNil() + } + region.MarshalEasyJSON(out) +} + // @Tags region // @Summary List all regions in the cluster. // @Produce json diff --git a/tools/pd-ctl/tests/region/region_test.go b/tools/pd-ctl/tests/region/region_test.go index ff6ab1a25f2..90a05ce6f91 100644 --- a/tools/pd-ctl/tests/region/region_test.go +++ b/tools/pd-ctl/tests/region/region_test.go @@ -208,3 +208,56 @@ func TestRegion(t *testing.T) { {core.HexRegionKeyStr(r5.GetEndKey()), ""}, }, *rangeHoles) } + +func TestRegionNoLeader(t *testing.T) { + re := require.New(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + cluster, err := pdTests.NewTestCluster(ctx, 1) + re.NoError(err) + err = cluster.RunInitialServers() + re.NoError(err) + cluster.WaitLeader() + url := cluster.GetConfig().GetClientURL() + stores := []*metapb.Store{ + { + Id: 1, + State: metapb.StoreState_Up, + LastHeartbeat: time.Now().UnixNano(), + }, + { + Id: 2, + State: metapb.StoreState_Up, + LastHeartbeat: time.Now().UnixNano(), + }, + { + Id: 3, + State: metapb.StoreState_Up, + LastHeartbeat: time.Now().UnixNano(), + }, + } + + leaderServer := cluster.GetLeaderServer() + re.NoError(leaderServer.BootstrapCluster()) + for i := 0; i < len(stores); i++ { + pdTests.MustPutStore(re, cluster, stores[i]) + } + + metaRegion := &metapb.Region{ + Id: 100, + StartKey: []byte(""), + EndKey: []byte(""), + Peers: []*metapb.Peer{ + {Id: 1, StoreId: 1}, + {Id: 5, StoreId: 2}, + {Id: 6, StoreId: 3}}, + RegionEpoch: &metapb.RegionEpoch{ConfVer: 1, Version: 1}, + } + r := core.NewRegionInfo(metaRegion, nil) + + cluster.GetLeaderServer().GetRaftCluster().GetBasicCluster().SetRegion(r) + + cmd := ctl.GetRootCmd() + _, err = tests.ExecuteCommand(cmd, "-u", url, "region", "100") + re.NoError(err) +}