go/zodb/internal/weak: Disable support for weak references

Experience shows that we cannot provide reliably working weak references
support without crashing GC because there needs to be a coordination in
between object resurrection and at least GC mark phase[1,2] with [3] and
[4] showing an idea what kind it coordination it needs to be.

And since support for weak references is upcoming in standard library
with hopefully Go 1.24 [5], it does not make sense to continue pushing
hard to make weak references work on our standalone side.

By disabling support for custom weak references we will avoid crashes,
but will start to leak unused objects in zodb.Connection live cache.
This should be relatively ok for now as WCFS in wendelin.core is
currently the only known ZODB/go user and it cares to drop ZBlk* state
after loading the data. The leak, thus, should be modest with,
hopefully, not creating problems in practice.

And once std package weak is out there we will switch to that restoring
automatic LiveCache cleanup.

[1] https://github.com/golang/go/issues/41303
[2] wendelin.core@9b44fc23
[3] https://github.com/golang/go/commit/dfc86e922cd0
[4] https://github.com/golang/go/commit/79fd633632cd
[5] https://github.com/golang/go/issues/67552

/reviewed-by @levin.zimmermann
/reviewed-on !11

go/zodb/connection.go

@@ -118,6 +118,11 @@ 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.
+	//
+	// 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.

go/zodb/internal/weak/weak.go

-// Copyright (C) 2018-2024  Nexedi SA and Contributors.
+// Copyright (C) 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 bug for intricate troubles of original implementation:
-// https://github.com/golang/go/issues/41303
-//
 // 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.
@@ -24,99 +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.
+//go:build !WEAK_BUGGY_CRASHES_GC
+
+// Package weak is stub for weak references.
 //
-// Note: this package should be superceeded with std package weak when
-// proposal for that gets implemented: https://github.com/golang/go/issues/67552.
+// It provides weak references API, but the references are full strong, not weak, pointers.
+//
+// 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
 
-import (
-	"fmt"
-	"runtime"
-	"sync"
-	"unsafe"
-
-	_ "go4.org/unsafe/assume-no-moving-gc"
-)
-
-
-// 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
-)
-
+const Stub = true
 
-// 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
+	ptr *T
 }
 
-// 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
+	return &Ref[T]{obj}
 }
 
-// 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
+	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

@@ -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 (

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)
+	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)
+	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)
 
+	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
 }