fix: Go code review — 7 critical issues resolved
From senior Go developer review: C7 CRITICAL: pagePool data race (ntx.go) - Moved global pagePool[8] + pagePoolIdx into per-Index struct - Eliminates race condition across goroutines using separate indexes C8 CRITICAL: Page.data dangling pointer after remap (ntx.go) - remapFile() now clears pagePool data slices (pointed into old mmap) - Prevents segfault from stale mmap references C4 HIGH: pop() bounds check restored (thread.go) - Removed performance optimization that eliminated underflow detection - Stack underflow now produces clear error instead of index -1 panic C1 HIGH: intExpLen overflow on MinInt64 (value.go) - Added special case: MinInt64 returns 20 (length of -9223372036854775808) - Prevents -v overflow in negation C11 CRITICAL: GoTo ReadAt error handling (dbf.go) - ReadAt failure now returns error and sets EOF - Previously silently used stale record buffer (data corruption risk) C14 HIGH: LEN() inline missing Hash case (gengo.go) - Added _v.IsHash() → len(Keys) branch C15 HIGH: EMPTY() inline missing Date case (gengo.go) - Added _v.IsDate() && _v.AsJulian() == 0 check 82/82 stress PASS. 14 packages ALL PASS. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1605,13 +1605,13 @@ func (g *Generator) tryEmitInlineRTL(name string, args []ast.Expr) bool {
|
||||
case "LEN":
|
||||
if len(args) == 1 {
|
||||
g.emitExpr(args[0])
|
||||
g.writeln("{ _v := t.Pop2(); if _v.IsString() { t.PushInt(len(_v.AsString())) } else if _v.IsArray() { t.PushInt(len(_v.AsArray().Items)) } else { t.PushInt(0) } }")
|
||||
g.writeln("{ _v := t.Pop2(); if _v.IsString() { t.PushInt(len(_v.AsString())) } else if _v.IsArray() { t.PushInt(len(_v.AsArray().Items)) } else if _v.IsHash() { t.PushInt(len(_v.AsHash().Keys)) } else { t.PushInt(0) } }")
|
||||
return true
|
||||
}
|
||||
case "EMPTY":
|
||||
if len(args) == 1 {
|
||||
g.emitExpr(args[0])
|
||||
g.writeln("{ _v := t.Pop2(); t.PushBool(_v.IsNil() || (_v.IsString() && len(strings.TrimSpace(_v.AsString())) == 0) || (_v.IsNumeric() && _v.AsNumDouble() == 0) || (_v.IsLogical() && !_v.AsBool())) }")
|
||||
g.writeln("{ _v := t.Pop2(); t.PushBool(_v.IsNil() || (_v.IsString() && len(strings.TrimSpace(_v.AsString())) == 0) || (_v.IsNumeric() && _v.AsNumDouble() == 0) || (_v.IsLogical() && !_v.AsBool()) || (_v.IsDate() && _v.AsJulian() == 0)) }")
|
||||
g.imports["strings"] = true
|
||||
return true
|
||||
}
|
||||
|
||||
@@ -401,8 +401,9 @@ func (a *DBFArea) GoTo(recNo uint32) error {
|
||||
recLen := int(a.header.RecordLen)
|
||||
if a.mmapData != nil && int(offset)+recLen <= len(a.mmapData) {
|
||||
copy(a.recBuf, a.mmapData[offset:offset+int64(recLen)])
|
||||
} else {
|
||||
a.dataFile.ReadAt(a.recBuf, offset)
|
||||
} else if _, err := a.dataFile.ReadAt(a.recBuf, offset); err != nil {
|
||||
a.FEof = true
|
||||
return fmt.Errorf("read record %d: %w", recNo, err)
|
||||
}
|
||||
|
||||
a.recNo = recNo
|
||||
|
||||
@@ -108,13 +108,11 @@ type Page struct {
|
||||
changed bool
|
||||
}
|
||||
|
||||
// pagePool reuses Page structs to reduce GC pressure.
|
||||
var pagePool [8]Page
|
||||
var pagePoolIdx int
|
||||
|
||||
func acquirePage() *Page {
|
||||
p := &pagePool[pagePoolIdx%len(pagePool)]
|
||||
pagePoolIdx++
|
||||
// acquirePage returns a reusable Page from the per-Index pool.
|
||||
// Pool is per-Index (not global) to avoid data races across goroutines.
|
||||
func (idx *Index) acquirePage() *Page {
|
||||
p := &idx.pagePool[idx.pagePoolIdx%len(idx.pagePool)]
|
||||
idx.pagePoolIdx++
|
||||
return p
|
||||
}
|
||||
|
||||
@@ -131,7 +129,7 @@ func LoadPage(f *os.File, offset int64) (*Page, error) {
|
||||
// cachedLoadPage — BoltDB-style zero-copy: returns slice into mmap.
|
||||
// No 1024-byte copy. Page.data points directly into mmap memory.
|
||||
func (idx *Index) cachedLoadPage(offset int64) (*Page, error) {
|
||||
p := acquirePage()
|
||||
p := idx.acquirePage()
|
||||
p.offset = offset
|
||||
|
||||
if idx.mmapData != nil && offset >= 0 && int(offset)+BlockSize <= len(idx.mmapData) {
|
||||
@@ -224,6 +222,10 @@ type Index struct {
|
||||
ascendKey bool
|
||||
uniqueKey bool
|
||||
keyType byte // 'C', 'N', 'D', 'L'
|
||||
|
||||
// Per-index page pool (avoids global state / data race)
|
||||
pagePool [8]Page
|
||||
pagePoolIdx int
|
||||
}
|
||||
|
||||
// OpenIndex opens an existing NTX index file.
|
||||
@@ -275,11 +277,16 @@ func (idx *Index) mmapFile() {
|
||||
}
|
||||
|
||||
// remapFile re-maps after file size changed (e.g., after insert/split).
|
||||
// Also invalidates page pool (data slices pointed into old mmap).
|
||||
func (idx *Index) remapFile() {
|
||||
if idx.mmapData != nil {
|
||||
syscall.Munmap(idx.mmapData)
|
||||
idx.mmapData = nil
|
||||
}
|
||||
// Invalidate page pool — data slices pointed into old mmap
|
||||
for i := range idx.pagePool {
|
||||
idx.pagePool[i].data = nil
|
||||
}
|
||||
idx.mmapFile()
|
||||
}
|
||||
|
||||
|
||||
@@ -123,9 +123,12 @@ func (t *Thread) push(v Value) {
|
||||
}
|
||||
|
||||
func (t *Thread) pop() Value {
|
||||
if t.sp <= 0 {
|
||||
panic(t.runtimeError("stack underflow"))
|
||||
}
|
||||
t.sp--
|
||||
v := t.stack[t.sp]
|
||||
t.stack[t.sp] = cachedNil // help GC (no alloc)
|
||||
t.stack[t.sp] = cachedNil
|
||||
return v
|
||||
}
|
||||
|
||||
|
||||
@@ -392,6 +392,9 @@ func intExpLen(v int64) int {
|
||||
n := 0
|
||||
if v < 0 {
|
||||
n = 1
|
||||
if v == math.MinInt64 {
|
||||
return 20 // "-9223372036854775808"
|
||||
}
|
||||
v = -v
|
||||
}
|
||||
for v > 0 {
|
||||
|
||||
Reference in New Issue
Block a user