Skip to content

Commit

Permalink
[BUGFIX] resolve agent GetGraphStatistics API double-free error problem
Browse files Browse the repository at this point in the history
Signed-off-by: kpango <kpango@vdaas.org>
  • Loading branch information
kpango committed Nov 5, 2024
1 parent a583f09 commit a9ce087
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 43 deletions.
103 changes: 64 additions & 39 deletions internal/core/algorithm/ngt/ngt.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/vdaas/vald/internal/file"
"github.com/vdaas/vald/internal/log"
"github.com/vdaas/vald/internal/sync"
"github.com/vdaas/vald/internal/sync/singleflight"
)

type (
Expand Down Expand Up @@ -85,7 +86,7 @@ type (
// GetVector returns vector stored in NGT index.
GetVector(id uint) ([]float32, error)

GetGraphStatistics(m statisticsType) (stats *GraphStatistics, err error)
GetGraphStatistics(ctx context.Context, m statisticsType) (stats *GraphStatistics, err error)

// GetProperty returns NGT Index Property.
GetProperty() (*Property, error)
Expand Down Expand Up @@ -114,8 +115,10 @@ type (
epl uint64 // NGT error buffer pool size limit
index C.NGTIndex
ospace C.NGTObjectSpace
group singleflight.Group[*GraphStatistics]
mu *sync.RWMutex
cmu *sync.RWMutex
smu *sync.Mutex
}

ngtError struct {
Expand Down Expand Up @@ -479,6 +482,7 @@ func gen(isLoad bool, opts ...Option) (NGT, error) {
)
n.mu = new(sync.RWMutex)
n.cmu = new(sync.RWMutex)
n.smu = new(sync.Mutex)

defer func() {
if err != nil {
Expand Down Expand Up @@ -1079,36 +1083,39 @@ func (n *ngt) Close() {
}

func fromCGraphStatistics(cstats *C.NGTGraphStatistics) *GraphStatistics {
if cstats == nil {
return nil
}

Check warning on line 1088 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1086-L1088

Added lines #L1086 - L1088 were not covered by tests
goStats := &GraphStatistics{
NumberOfObjects: uint64(cstats.numberOfObjects),
NumberOfIndexedObjects: uint64(cstats.numberOfIndexedObjects),
SizeOfObjectRepository: uint64(cstats.sizeOfObjectRepository),
SizeOfRefinementObjectRepository: uint64(cstats.sizeOfRefinementObjectRepository),
NumberOfRemovedObjects: uint64(cstats.numberOfRemovedObjects),
NumberOfNodes: uint64(cstats.numberOfNodes),
NumberOfEdges: uint64(cstats.numberOfEdges),
C1Indegree: float64(cstats.c1Indegree),
C5Indegree: float64(cstats.c5Indegree),
C95Outdegree: float64(cstats.c95Outdegree),
C99Outdegree: float64(cstats.c99Outdegree),
MaxNumberOfIndegree: uint64(cstats.maxNumberOfIndegree),
MaxNumberOfOutdegree: uint64(cstats.maxNumberOfOutdegree),

Check warning on line 1095 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1090-L1095

Added lines #L1090 - L1095 were not covered by tests
MeanEdgeLength: float64(cstats.meanEdgeLength),
MeanEdgeLengthFor10Edges: float64(cstats.meanEdgeLengthFor10Edges),
MeanIndegreeDistanceFor10Edges: float64(cstats.meanIndegreeDistanceFor10Edges),

Check warning on line 1098 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1097-L1098

Added lines #L1097 - L1098 were not covered by tests
MeanNumberOfEdgesPerNode: float64(cstats.meanNumberOfEdgesPerNode),
NumberOfNodesWithoutEdges: uint64(cstats.numberOfNodesWithoutEdges),
MaxNumberOfOutdegree: uint64(cstats.maxNumberOfOutdegree),
MinNumberOfOutdegree: uint64(cstats.minNumberOfOutdegree),
NumberOfNodesWithoutIndegree: uint64(cstats.numberOfNodesWithoutIndegree),
MaxNumberOfIndegree: uint64(cstats.maxNumberOfIndegree),
MedianIndegree: int32(cstats.medianIndegree),
MedianOutdegree: int32(cstats.medianOutdegree),

Check warning on line 1101 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1100-L1101

Added lines #L1100 - L1101 were not covered by tests
MinNumberOfIndegree: uint64(cstats.minNumberOfIndegree),
MeanEdgeLengthFor10Edges: float64(cstats.meanEdgeLengthFor10Edges),
MinNumberOfOutdegree: uint64(cstats.minNumberOfOutdegree),
ModeIndegree: uint64(cstats.modeIndegree),
ModeOutdegree: uint64(cstats.modeOutdegree),

Check warning on line 1105 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1103-L1105

Added lines #L1103 - L1105 were not covered by tests
NodesSkippedFor10Edges: uint64(cstats.nodesSkippedFor10Edges),
MeanIndegreeDistanceFor10Edges: float64(cstats.meanIndegreeDistanceFor10Edges),
NodesSkippedForIndegreeDistance: uint64(cstats.nodesSkippedForIndegreeDistance),
VarianceOfOutdegree: float64(cstats.varianceOfOutdegree),
NumberOfEdges: uint64(cstats.numberOfEdges),
NumberOfIndexedObjects: uint64(cstats.numberOfIndexedObjects),
NumberOfNodes: uint64(cstats.numberOfNodes),
NumberOfNodesWithoutEdges: uint64(cstats.numberOfNodesWithoutEdges),
NumberOfNodesWithoutIndegree: uint64(cstats.numberOfNodesWithoutIndegree),
NumberOfObjects: uint64(cstats.numberOfObjects),
NumberOfRemovedObjects: uint64(cstats.numberOfRemovedObjects),
SizeOfObjectRepository: uint64(cstats.sizeOfObjectRepository),
SizeOfRefinementObjectRepository: uint64(cstats.sizeOfRefinementObjectRepository),

Check warning on line 1116 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1108-L1116

Added lines #L1108 - L1116 were not covered by tests
VarianceOfIndegree: float64(cstats.varianceOfIndegree),
MedianOutdegree: int32(cstats.medianOutdegree),
ModeOutdegree: uint64(cstats.modeOutdegree),
C95Outdegree: float64(cstats.c95Outdegree),
C99Outdegree: float64(cstats.c99Outdegree),
MedianIndegree: int32(cstats.medianIndegree),
ModeIndegree: uint64(cstats.modeIndegree),
C5Indegree: float64(cstats.c5Indegree),
C1Indegree: float64(cstats.c1Indegree),
VarianceOfOutdegree: float64(cstats.varianceOfOutdegree),

Check warning on line 1118 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1118

Added line #L1118 was not covered by tests
Valid: bool(cstats.valid),
}

Expand Down Expand Up @@ -1139,22 +1146,40 @@ func fromCGraphStatistics(cstats *C.NGTGraphStatistics) *GraphStatistics {
return goStats
}

func (n *ngt) GetGraphStatistics(m statisticsType) (stats *GraphStatistics, err error) {
var mode rune
switch m {
case NormalStatistics:
mode = '-'
case AdditionalStatistics:
mode = 'a'
}
ne := n.GetErrorBuffer()
cstats := C.ngt_get_graph_statistics(n.index, C.char(mode), C.size_t(n.ces), ne.err)
if !cstats.valid {
return nil, n.newGoError(ne)
func (n *ngt) GetGraphStatistics(
ctx context.Context, m statisticsType,
) (stats *GraphStatistics, err error) {
var shared bool
stats, shared, err = n.group.Do(ctx, "GetGraphStatistics", func(context.Context) (*GraphStatistics, error) {
n.smu.Lock()
defer n.smu.Unlock()
var mode rune
switch m {
case NormalStatistics:
mode = '-'
case AdditionalStatistics:
mode = 'a'

Check warning on line 1161 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1151-L1161

Added lines #L1151 - L1161 were not covered by tests
}
ne := n.GetErrorBuffer()
cstats := C.ngt_get_graph_statistics(n.index, C.char(mode), C.size_t(n.ces), ne.err)
if !cstats.valid {
return nil, n.newGoError(ne)
}
n.PutErrorBuffer(ne)
defer C.ngt_free_graph_statistics(&cstats)
s := fromCGraphStatistics(&cstats)
if s == nil {
return nil, errors.ErrNGTIndexStatisticsNotReady
}
return s, nil

Check warning on line 1174 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1163-L1174

Added lines #L1163 - L1174 were not covered by tests
})
if err != nil {
if shared && !errors.Is(err, errors.ErrNGTIndexStatisticsNotReady) {
return n.GetGraphStatistics(ctx, m)
}
return nil, err

Check warning on line 1180 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1176-L1180

Added lines #L1176 - L1180 were not covered by tests
}
n.PutErrorBuffer(ne)
defer C.ngt_free_graph_statistics(&cstats)
return fromCGraphStatistics(&cstats), nil
return stats, nil

Check warning on line 1182 in internal/core/algorithm/ngt/ngt.go

View check run for this annotation

Codecov / codecov/patch

internal/core/algorithm/ngt/ngt.go#L1182

Added line #L1182 was not covered by tests
}

func (n *ngt) GetProperty() (prop *Property, err error) {
Expand Down
8 changes: 4 additions & 4 deletions pkg/agent/core/ngt/service/ngt.go
Original file line number Diff line number Diff line change
Expand Up @@ -1306,7 +1306,7 @@ func (n *ngt) RegenerateIndexes(ctx context.Context) (err error) {
}
n.copyNGT(nn)

return n.loadStatistics()
return n.loadStatistics(ctx)

Check warning on line 1309 in pkg/agent/core/ngt/service/ngt.go

View check run for this annotation

Codecov / codecov/patch

pkg/agent/core/ngt/service/ngt.go#L1309

Added line #L1309 was not covered by tests
}

func (n *ngt) CreateIndex(ctx context.Context, poolSize uint32) (err error) {
Expand Down Expand Up @@ -1450,13 +1450,13 @@ func (n *ngt) CreateIndex(ctx context.Context, poolSize uint32) (err error) {
return err
}
}
return n.loadStatistics()
return n.loadStatistics(ctx)
}

func (n *ngt) loadStatistics() error {
func (n *ngt) loadStatistics(ctx context.Context) error {
if n.IsStatisticsEnabled() {
log.Info("loading index statistics to cache")
stats, err := n.core.GetGraphStatistics(core.AdditionalStatistics)
stats, err := n.core.GetGraphStatistics(ctx, core.AdditionalStatistics)

Check warning on line 1459 in pkg/agent/core/ngt/service/ngt.go

View check run for this annotation

Codecov / codecov/patch

pkg/agent/core/ngt/service/ngt.go#L1459

Added line #L1459 was not covered by tests
if err != nil {
log.Errorf("failed to load index statistics to cache: %v", err)
return err
Expand Down

0 comments on commit a9ce087

Please sign in to comment.