Merge branch 'master' into t

This replaces previous attempts to fix zodb/internal/weak with patches
from master.

* master:
  go/zodb/internal/weak: Disable support for weak references
  go/zodb/internal/weak: Assert that the object was not moved in the finalizer
  go/zodb/internal/weak: Try to fix GC crashes via reworking Ref to keep only one word instead of two

go/zodb/connection.go

-// Copyright (C) 2018-2021  Nexedi SA and Contributors.
+// Copyright (C) 2018-2024  Nexedi SA and Contributors.
 //                          Kirill Smelkov <kirr@nexedi.com>
 //
 // This program is free software: you can Use, Study, Modify and Redistribute
@@ -118,7 +118,12 @@ type LiveCache struct {
 	//
 	// NOTE2 finalizers don't run on when they are attached to an object in cycle.
 	// Hopefully we don't have cycles with BTree/Bucket.
-	objtab map[Oid]*weak.Ref // oid -> weak.Ref(IPersistent)
+	//
+	// FIXME currently zodb/internal/weak only pretends it supports weak
+	//       references and uses strong pointers instead. We are waiting
+	//       for https://github.com/golang/go/issues/67552 to fix that
+	//       with weak package from Go standard library.
+	objtab map[Oid]*weak.Ref[Persistent]
 
 	// hooks for application to influence live caching decisions.
 	control LiveCacheControl
@@ -185,7 +190,7 @@ func newConnection(db *DB, at Tid) *Connection {
 		at:    at,
 		cache: LiveCache{
 			pinned: make(map[Oid]IPersistent),
-			objtab: make(map[Oid]*weak.Ref),
+			objtab: make(map[Oid]*weak.Ref[Persistent]),
 		},
 	}
 	if cc := db.opt.CacheControl; cc != nil {
@@ -236,7 +241,7 @@ func (cache *LiveCache) Get(oid Oid) IPersistent {
 	wobj := cache.objtab[oid]
 	if wobj != nil {
 		if xobj := wobj.Get(); xobj != nil {
-			obj = xobj.(IPersistent)
+			obj = xobj.instance
 		}
 	}
 
@@ -257,7 +262,7 @@ func (cache *LiveCache) setNew(oid Oid, obj IPersistent) {
 		cache.pinned[oid] = obj
 		// XXX assert .objtab[oid] == nil ?
 	} else {
-		cache.objtab[oid] = weak.NewRef(obj)
+		cache.objtab[oid] = weak.NewRef(obj.persistent())
 		// XXX assert .pinned[oid] == nil ?
 	}
 }
@@ -269,7 +274,7 @@ func (cache *LiveCache) forEach(f func(IPersistent)) {
 	}
 	for _, wobj := range cache.objtab {
 		if xobj := wobj.Get(); xobj != nil {
-			f(xobj.(IPersistent))
+			f(xobj.instance)
 		}
 	}
 }
@@ -285,7 +290,7 @@ func (cache *LiveCache) SetControl(c LiveCacheControl) {
 
 	// reclassify all objects
 	c2 := *cache
-	cache.objtab = make(map[Oid]*weak.Ref)
+	cache.objtab = make(map[Oid]*weak.Ref[Persistent])
 	cache.pinned = make(map[Oid]IPersistent)
 	c2.forEach(func(obj IPersistent) {
 		cache.setNew(obj.POid(), obj)

go/zodb/internal/weak/weak.go

-// Copyright (C) 2018-2020  Nexedi SA and Contributors.
-//                          Kirill Smelkov <kirr@nexedi.com>
-//
-// based on:
-// https://groups.google.com/d/msg/golang-nuts/PYWxjT2v6ps/dL71oJk1mXEJ
-// https://play.golang.org/p/f9HY6-z8Pp
+// Copyright (C) 2024  Nexedi SA and Contributors.
+//                     Kirill Smelkov <kirr@nexedi.com>
 //
 // This program is free software: you can Use, Study, Modify and Redistribute
 // it under the terms of the GNU General Public License version 3, or (at your
@@ -21,126 +17,26 @@
 // See COPYING file for full licensing terms.
 // See https://www.nexedi.com/licensing for rationale and options.
 
-// Package weak provides weak references for Go.
-package weak
-
-//go:generate gotrace gen .
+//go:build !WEAK_BUGGY_CRASHES_GC
 
-import (
-	"fmt"
-	"runtime"
-	"sync"
-//	"time"
-	"unsafe"
-)
-
-// iface is how Go runtime represents an interface.
-//
-// NOTE layout must be synchronized to Go runtime representation.
-// NOTE correctness depends on non-moving property of Go GC.
-type iface struct {
-	typ  uintptr // type
-	data uintptr // data
-}
-
-// weakRefState represents current state of an object Ref points to.
-type weakRefState int32
-
-const (
-	objGot      weakRefState = +1 // Ref.Get returned !nil
-	objLive     weakRefState =  0 // object is alive, Get did not run yet in this GC cycle
-	objReleased weakRefState = -1 // the finalizer marked object as released
-)
-
-
-// Ref is a weak reference.
-//
-// Create one with NewRef and retrieve referenced object with Get.
+// Package weak is stub for weak references.
 //
-// There must be no more than 1 weak reference to any object.
-// Weak references must not be attached to an object on which runtime.SetFinalizer is also used.
-// Weak references must not be copied.
-type Ref struct {
-	iface
-
-	// XXX try to do without mutex and only with atomics
-	mu    sync.Mutex
-	state weakRefState
-}
-
-//trace:event traceRelease(w *Ref)
-
-// NewRef creates new weak reference pointing to obj.
+// It provides weak references API, but the references are full strong, not weak, pointers.
 //
-// TODO + onrelease callback?
-func NewRef(obj interface{}) *Ref {
-	// since starting from ~ Go1.4 the GC is precise, we can save interface
-	// pointers to uintptr and that won't prevent GC from garbage
-	// collecting the object.
-	w := &Ref{
-		iface: *(*iface)(unsafe.Pointer(&obj)),
-		state: objLive,
-	}
-
-	var release func(interface{})
-	release = func(obj interface{}) {
-		// assert that the object was not moved
-		iobj := *(*iface)(unsafe.Pointer(&obj))
-		if w.iface != iobj {
-			panic(fmt.Sprintf("weak: release: object moved:  w.iface=%x  obj=%x", w.iface, iobj))
-		}
-
-		// GC decided that the object is no longer reachable and
-		// scheduled us to run as finalizer. During the time till we
-		// actually run, Ref.Get might have been come to run and
-		// "rematerializing" the object for use. Check if we do not
-		// race with any Get in progress, and reschedule us to retry at
-		// next GC if we do.
-		w.mu.Lock()
-		//old := w.state
-		if w.state == objGot {
-			w.state = objLive
-			runtime.SetFinalizer(obj, release)
-		} else {
-			w.state = objReleased
-		}
-		//fmt.Printf("rel %p (state: %v -> %v)\n", w, old, w.state)
-		traceRelease(w)
-		w.mu.Unlock()
+// It should be removed once std package weak is, hopefully, provided in Go standard library.
+// See https://github.com/golang/go/issues/67552 for details.
+package weak
 
-	}
+const Stub = true
 
-	runtime.SetFinalizer(obj, release)
-	return w
+type Ref[T any] struct {
+	ptr *T
 }
 
-//trace:event traceGotPre(w *Ref)
-
-// Get returns object pointed to by this weak reference.
-//
-// If original object is still alive - it is returned.
-// If not - nil is returned.
-func (w *Ref) Get() (obj interface{}) {
-	w.mu.Lock()
-	if w.state != objReleased {
-		//fmt.Printf("got %p (state: %v -> %v)\n", w, w.state, objGot)
-		w.state = objGot
-
-		traceGotPre(w)
-		// XXX sleep causes `panic: non-empty mark queue after concurrent mark` when running TestΔBTail in wcfs
-		// see https://github.com/golang/go/issues/41303
-		//time.Sleep(100*time.Nanosecond)
-		//time.Sleep(10*time.Millisecond)
-		//runtime.GC()
-		//runtime.GC()
-		//
-		// see also: https://github.com/golang/go/issues/43615
+func NewRef[T any](obj *T) *Ref[T] {
+	return &Ref[T]{obj}
+}
 
-		// recreate interface{} from saved words.
-		// XXX do writes as pointers so that compiler emits write barriers to notify GC?
-		i := (*iface)(unsafe.Pointer(&obj))
-		*i = w.iface
-	}
-	w.mu.Unlock()
-	return obj
+func (w *Ref[T]) Get() (obj *T) {
+	return w.ptr
 }

go/zodb/internal/weak/weak_buggy.go

+// Copyright (C) 2018-2024  Nexedi SA and Contributors.
+//                          Kirill Smelkov <kirr@nexedi.com>
+//
+// initially based on:
+// https://groups.google.com/d/msg/golang-nuts/PYWxjT2v6ps/dL71oJk1mXEJ
+// https://play.golang.org/p/f9HY6-z8Pp
+//
+// see the following references for intricate troubles of original and current implementations:
+// https://github.com/golang/go/issues/41303
+// https://lab.nexedi.com/kirr/wendelin.core/-/commit/9b44fc23
+//
+// This program is free software: you can Use, Study, Modify and Redistribute
+// it under the terms of the GNU General Public License version 3, or (at your
+// option) any later version, as published by the Free Software Foundation.
+//
+// You can also Link and Combine this program with other software covered by
+// the terms of any of the Free Software licenses or any of the Open Source
+// Initiative approved licenses and Convey the resulting work. Corresponding
+// source of such a combination shall include the source code for all other
+// software used.
+//
+// This program is distributed WITHOUT ANY WARRANTY; without even the implied
+// warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+//
+// See COPYING file for full licensing terms.
+// See https://www.nexedi.com/licensing for rationale and options.
+
+//go:build WEAK_BUGGY_CRASHES_GC
+
+// Package weak provides weak references for Go.
+//
+// FIXME this package is buggy and should be superceeded with std package weak when
+// proposal for that gets implemented: https://github.com/golang/go/issues/67552.
+package weak
+
+import (
+	"fmt"
+	"runtime"
+	"sync"
+	"unsafe"
+
+	_ "go4.org/unsafe/assume-no-moving-gc"
+)
+
+const Stub = false
+
+// weakRefState represents current state of an object Ref points to.
+type weakRefState int32
+
+const (
+	objGot      weakRefState = +1 // Ref.Get returned !nil
+	objLive     weakRefState =  0 // object is alive, Get did not run yet in this GC cycle
+	objReleased weakRefState = -1 // the finalizer marked object as released
+)
+
+
+// Ref[T] is a weak reference.
+//
+// Create one with NewRef and retrieve referenced object with Get.
+//
+// There must be no more than 1 weak reference to any object.
+// Weak references must not be attached to an object on which runtime.SetFinalizer is also used.
+// Weak references must not be copied.
+type Ref[T any] struct {
+	iptr uintptr
+
+	// XXX try to do without mutex and only with atomics
+	mu    sync.Mutex
+	state weakRefState
+}
+
+// NewRef creates new weak reference pointing to obj.
+//
+// TODO + onrelease callback?
+func NewRef[T any](obj *T) *Ref[T] {
+	// since starting from ~ Go1.4 the GC is precise, we can save interface
+	// pointers to uintptr and that won't prevent GC from garbage
+	// collecting the object.
+	w := &Ref[T]{
+		iptr:  (uintptr)(unsafe.Pointer(obj)),
+		state: objLive,
+	}
+
+	var release func(*T)
+	release = func(obj *T) {
+		// assert that the object was not moved
+		iptr := (uintptr)(unsafe.Pointer(obj))
+		if w.iptr != iptr {
+			panic(fmt.Sprintf("weak: release: object moved:  w.iptr=%x  obj=%x", w.iptr, iptr))
+		}
+
+		// GC decided that the object is no longer reachable and
+		// scheduled us to run as finalizer. During the time till we
+		// actually run, Ref.Get might have been come to run and
+		// "rematerializing" the object for use. Check if we do not
+		// race with any Get in progress, and reschedule us to retry at
+		// next GC if we do.
+		w.mu.Lock()
+		if w.state == objGot {
+			w.state = objLive
+			runtime.SetFinalizer(obj, release)
+		} else {
+			w.state = objReleased
+		}
+		w.mu.Unlock()
+	}
+
+	runtime.SetFinalizer(obj, release)
+	return w
+}
+
+// Get returns object pointed to by this weak reference.
+//
+// If original object is still alive - it is returned.
+// If not - nil is returned.
+func (w *Ref[T]) Get() (obj *T) {
+	w.mu.Lock()
+	if w.state != objReleased {
+		w.state = objGot
+
+		// recreate pointer from saved word.
+		obj = (*T)(unsafe.Pointer(w.iptr))
+	}
+	w.mu.Unlock()
+	return obj
+}

go/zodb/internal/weak/weak_test.go → go/zodb/internal/weak/weak_buggy_test.go

-// Copyright (C) 2018-2020  Nexedi SA and Contributors.
+// Copyright (C) 2018-2024  Nexedi SA and Contributors.
 //                          Kirill Smelkov <kirr@nexedi.com>
 //
 // This program is free software: you can Use, Study, Modify and Redistribute
@@ -17,6 +17,8 @@
 // See COPYING file for full licensing terms.
 // See https://www.nexedi.com/licensing for rationale and options.
 
+//go:build WEAK_BUGGY_CRASHES_GC
+
 package weak
 
 import (
@@ -24,68 +26,15 @@ import (
 	"testing"
 	"time"
 	"unsafe"
-
-	"lab.nexedi.com/kirr/go123/tracing"
 )
 
-// verify that interface <-> iface works ok.
-func TestIface(t *testing.T) {
-	var i interface{}
-	var fi *iface
-
-	isize := unsafe.Sizeof(i)
-	fsize := unsafe.Sizeof(*fi)
-
-	if isize != fsize {
-		t.Fatalf("sizeof(interface{}) (%d)  !=   sizeof(iface) (%d)", isize, fsize)
-	}
-
-	i = 3
-	var j interface{}
-	if !(j == nil && i != j) {
-		t.Fatalf("i == j ?  (i: %#v,  j: %#v}", i, j)
-	}
-
-	fi = (*iface)(unsafe.Pointer(&i))
-	fj := (*iface)(unsafe.Pointer(&j))
-	*fj = *fi
-
-	if i != j {
-		t.Fatalf("i (%#v)  !=  j (%#v)", i, j)
-	}
-}
-
-// FIXME currently fails.
 func TestWeakRef(t *testing.T) {
-	for i := 0; i < 100; i++ {
-		println(i)
-		testWeakRef(t)
-	}
-}
-
-func testWeakRef(t *testing.T) {
 	type T struct{ _ [8]int64 } // large enough not to go into tinyalloc
 
 	p := new(T)
 	w := NewRef(p)
 	pptr := uintptr(unsafe.Pointer(p))
 
-	wrelease := make(chan weakRefState) // w.state from traceRelease(w) event
-	tpg := &tracing.ProbeGroup{}
-	tracing.Lock()
-	traceRelease_Attach(tpg, func(w_ *Ref) {
-		if w_ != w {
-			panic("release: w != w_")
-		}
-		wrelease <- w.state
-	})
-	traceGotPre_Attach(tpg, func(w *Ref) {
-		// nop for now
-		//panic("TODO GotPre")
-	})
-	tracing.Unlock()
-	defer tpg.Done()
-
 	assertEq := func(a, b interface{}) {
 		t.Helper()
 		if a != b {
@@ -94,59 +43,35 @@ func testWeakRef(t *testing.T) {
 	}
 
 	// perform GC + give finalizers a chance to run.
-	GCnofin := func() {
-		t.Helper()
-		runtime.GC()
-
-		select {
-		case <-time.After(10 * time.Millisecond):
-			// ok
-		case state := <-wrelease:
-			t.Fatalf("unexpected release event: state=%v", state)
-		}
-	}
-	GCfin := func(stateOK weakRefState) {
-		t.Helper()
+	GC := func() {
 		runtime.GC()
 
 		// GC only queues finalizers, not runs them directly. Give it
 		// some time so that finalizers could have been run.
-		var state weakRefState
-		select {
-		case state = <-wrelease:
-			// ok
-		case <-time.After(1 * time.Second):
-			t.Fatalf("no release event  (w.state=%v  stateOK=%v)", w.state, stateOK)
-		}
-
-		if state != stateOK {
-			t.Fatalf("release: state != stateOK;  state=%v  stateOK=%v", state, stateOK)
-		}
+		time.Sleep(10 * time.Millisecond) // XXX hack
 	}
 
 	assertEq(w.state, objLive)
 	assertEq(w.Get(), p)
 	assertEq(w.state, objGot)
-	GCnofin()
+	GC()
 	assertEq(w.state, objGot) // fin has not been run at all (p is live)
 	assertEq(w.Get(), p)
 	assertEq(w.state, objGot)
 
 	p = nil
-	GCfin(objLive) // fin ran and downgraded got -> live
-	assertEq(w.state, objLive)
-	switch p_ := w.Get().(type) {
-	default:
-		t.Fatalf("Get after objGot -> objLive: %#v", p_)
-	case *T:
-		if uintptr(unsafe.Pointer(p_)) != pptr {
-			t.Fatal("Get after objGot -> objLive: T, but ptr is not the same")
-		}
+	GC()
+	assertEq(w.state, objLive) // fin ran and downgraded got -> live
+	p_ := w.Get()
+	if uintptr(unsafe.Pointer(p_)) != pptr {
+		t.Fatal("Get after objGot -> objLive: ptr is not the same")
 	}
 	assertEq(w.state, objGot)
 
-	GCfin(objLive) // fin ran again and again downgraded got -> live
+	GC()
+	assertEq(w.state, objLive) // fin ran again and again downgraded got -> live
 
-	GCfin(objReleased) // fin ran again and released the object
-	assertEq(w.Get(), nil)
+	GC()
+	assertEq(w.state, objReleased) // fin ran again and released the object
+	assertEq(w.Get(), (*T)(nil))
 }

go/zodb/internal/weak/ztrace.go

-// Code generated by lab.nexedi.com/kirr/go123/tracing/cmd/gotrace; DO NOT EDIT.
-
-package weak
-// code generated for tracepoints
-
-import (
-	"lab.nexedi.com/kirr/go123/tracing"
-	"unsafe"
-)
-
-// traceevent: traceGotPre(w *Ref)
-
-type _t_traceGotPre struct {
-	tracing.Probe
-	probefunc     func(w *Ref)
-}
-
-var _traceGotPre *_t_traceGotPre
-
-func traceGotPre(w *Ref) {
-	if _traceGotPre != nil {
-		_traceGotPre_run(w)
-	}
-}
-
-func _traceGotPre_run(w *Ref) {
-	for p := _traceGotPre; p != nil; p = (*_t_traceGotPre)(unsafe.Pointer(p.Next())) {
-		p.probefunc(w)
-	}
-}
-
-func traceGotPre_Attach(pg *tracing.ProbeGroup, probe func(w *Ref)) *tracing.Probe {
-	p := _t_traceGotPre{probefunc: probe}
-	tracing.AttachProbe(pg, (**tracing.Probe)(unsafe.Pointer(&_traceGotPre)), &p.Probe)
-	return &p.Probe
-}
-
-// traceevent: traceRelease(w *Ref)
-
-type _t_traceRelease struct {
-	tracing.Probe
-	probefunc     func(w *Ref)
-}
-
-var _traceRelease *_t_traceRelease
-
-func traceRelease(w *Ref) {
-	if _traceRelease != nil {
-		_traceRelease_run(w)
-	}
-}
-
-func _traceRelease_run(w *Ref) {
-	for p := _traceRelease; p != nil; p = (*_t_traceRelease)(unsafe.Pointer(p.Next())) {
-		p.probefunc(w)
-	}
-}
-
-func traceRelease_Attach(pg *tracing.ProbeGroup, probe func(w *Ref)) *tracing.Probe {
-	p := _t_traceRelease{probefunc: probe}
-	tracing.AttachProbe(pg, (**tracing.Probe)(unsafe.Pointer(&_traceRelease)), &p.Probe)
-	return &p.Probe
-}
-
-// trace export signature
-func _trace_exporthash_55416cf1cbcad92c4b4497269fa14b46e82469bd() {}

go/zodb/persistent_test.go

-// Copyright (C) 2018-2021  Nexedi SA and Contributors.
+// Copyright (C) 2018-2024  Nexedi SA and Contributors.
 //                          Kirill Smelkov <kirr@nexedi.com>
 //
 // This program is free software: you can Use, Study, Modify and Redistribute
@@ -30,6 +30,7 @@ import (
 	"testing"
 
 	"lab.nexedi.com/kirr/neo/go/transaction"
+	"lab.nexedi.com/kirr/neo/go/zodb/internal/weak"
 
 	"lab.nexedi.com/kirr/go123/mem"
 	assert "github.com/stretchr/testify/require"
@@ -887,10 +888,18 @@ func TestLiveCache(t0 *testing.T) {
 	xobjPK := zcache.Get(102)
 	xobjPD := zcache.Get(103)
 	xobjDD := zcache.Get(104)
-	assert.Equal(xobj, nil)
+	if !weak.Stub {
+		assert.Equal(xobj, nil)
+	} else {
+		assert.NotEqual(xobj, nil)   // stays in cache when weak implementation is stub
+	}
 	assert.NotEqual(xobjPK, nil)
 	assert.NotEqual(xobjPD, nil)
-	assert.Equal(xobjDD, nil)
+	if !weak.Stub {
+		assert.Equal(xobjDD, nil)
+	} else {
+		assert.NotEqual(xobjDD, nil)
+	}
 	objPK = xobjPK.(*MyObject)
 	objPD = xobjPD.(*MyObject)
 	t.checkObj(objPK, 102, at1, UPTODATE,      0, "труд")
@@ -904,8 +913,13 @@ func TestLiveCache(t0 *testing.T) {
 	t.checkObj(obj,   101, InvalidTid, GHOST,  0)
 	t.checkObj(objDD, 104, InvalidTid, GHOST,  0)
 
-	assert.Equal(obj._v_cookie, "")
-	assert.Equal(objDD._v_cookie, "")
+	if !weak.Stub {
+		assert.Equal(obj._v_cookie, "")
+		assert.Equal(objDD._v_cookie, "")
+	} else {
+		assert.Equal(obj._v_cookie, "peace")
+		assert.Equal(objDD._v_cookie, "spring")
+	}
 
 	// TODO reclassify tests
 }