WIP fix racy holds

fixes #280

daemon/control.go

@@ -147,8 +147,8 @@ func (j *controlJob) Run(ctx context.Context) {
 	server := http.Server{
 		Handler: mux,
 		// control socket is local, 1s timeout should be more than sufficient, even on a loaded system
-		WriteTimeout: envconst.Duration("ZREPL_DAEMON_CONTROL_WRITE_TIMEOUT", 1*time.Second),
-		ReadTimeout:  envconst.Duration("ZREPL_DAEMON_CONTROL_READ_TIMEOUT", 1*time.Second),
+		WriteTimeout: 1 * time.Second,
+		ReadTimeout:  1 * time.Second,
 	}
 
 outer:

endpoint/endpoint.go

@@ -198,11 +198,7 @@ func (s *Sender) Send(ctx context.Context, r *pdu.SendReq) (*pdu.SendRes, zfs.St
 	if err != nil {
 		return nil, nil, err
 	}
-	defer func(guardp **semaphore.AcquireGuard) {
-		if *guardp != nil {
-			(*guardp).Release()
-		}
-	}(&guard)
+	defer guard.Release()
 
 	si, err := zfs.ZFSSendDry(ctx, sendArgs)
 	if err != nil {
@@ -259,13 +255,6 @@ func (s *Sender) Send(ctx context.Context, r *pdu.SendReq) (*pdu.SendRes, zfs.St
 	if err != nil {
 		return nil, nil, errors.Wrap(err, "zfs send failed")
 	}
-
-	// defer releasing guard until streamCopier is closed
-	streamCopier.SetPostCloseCallback(func(_ error) {
-		guard.Release()
-	})
-	guard = nil
-
 	return res, streamCopier, nil
 }
 

endpoint/endpoint_zfs.go

@@ -411,6 +411,7 @@ func ListZFSHoldsAndBookmarks(ctx context.Context, fsfilter zfs.DatasetFilter) (
 	for _, fs := range fss {
 		err := listZFSHoldsAndBookmarksImplFS(ctx, out, fs)
 		if err != nil {
+			// FIXME if _, ok := err.(*zfs.DatasetDoesNotExist); ok { noop }
 			return nil, errors.Wrapf(err, "list holds and bookmarks on %q", fs.ToString())
 		}
 	}
@@ -420,7 +421,8 @@ func ListZFSHoldsAndBookmarks(ctx context.Context, fsfilter zfs.DatasetFilter) (
 func listZFSHoldsAndBookmarksImplFS(ctx context.Context, out *ListHoldsAndBookmarksOutput, fs *zfs.DatasetPath) error {
 	fsvs, err := zfs.ZFSListFilesystemVersions(fs, nil)
 	if err != nil {
-		return errors.Wrapf(err, "list filesystem versions of %q", fs)
+		// FIXME if _, ok := err.(*zfs.DatasetDoesNotExist); ok { noop }
+		return errors.Wrapf(err, "list filesystem versions of %q", fs.ToString())
 	}
 	for _, v := range fsvs {
 		switch v.Type {
@@ -429,7 +431,12 @@ func listZFSHoldsAndBookmarksImplFS(ctx context.Context, out *ListHoldsAndBookma
 		case zfs.Snapshot:
 			holds, err := zfs.ZFSHolds(ctx, fs.ToString(), v.Name)
 			if err != nil {
-				return errors.Wrapf(err, "get holds of %q", v.ToAbsPath(fs))
+				if _, ok := err.(*zfs.DatasetDoesNotExist); ok {
+					holds  = []string{}
+					// fallthrough
+				} else {
+					return errors.Wrapf(err, "get holds of %q", v.ToAbsPath(fs))
+				}
 			}
 			for _, tag := range holds {
 				listZFSHoldsAndBookmarksImplSnapshotTryParseHold(ctx, out, fs, v, tag)

platformtest/tests/idempotentHold.go

@@ -3,6 +3,7 @@ package tests
 import (
 	"fmt"
 
+	"github.com/stretchr/testify/require"
 	"github.com/zrepl/zrepl/platformtest"
 	"github.com/zrepl/zrepl/zfs"
 )
@@ -13,35 +14,28 @@ func IdempotentHold(ctx *platformtest.Context) {
 		DESTROYROOT
 		CREATEROOT
 		+  "foo bar"
-		+  "foo bar@1"
-	`)
-	defer platformtest.Run(ctx, platformtest.PanicErr, ctx.RootDataset, `
-		R zfs release zrepl_platformtest "${ROOTDS}/foo bar@1"
-		-  "foo bar@1"
-		-  "foo bar"
+		+  "foo bar@1 2"
 	`)
 
 	fs := fmt.Sprintf("%s/foo bar", ctx.RootDataset)
-	v1 := sendArgVersion(fs, "@1")
+	snap := sendArgVersion(fs, "@1 2")
 
 	tag := "zrepl_platformtest"
-	err := zfs.ZFSHold(ctx, fs, v1, tag)
+	err := zfs.ZFSHold(ctx, fs, snap, tag)
 	if err != nil {
 		panic(err)
 	}
 
-	err = zfs.ZFSHold(ctx, fs, v1, tag)
-	if err != nil {
-		panic(err)
-	}
-
-	vnonexistent := zfs.ZFSSendArgVersion{
-		RelName: "@nonexistent",
-		GUID:    0xbadf00d,
-	}
-	err = zfs.ZFSHold(ctx, fs, vnonexistent, tag)
-	if err == nil {
-		panic("still expecting error for nonexistent snapshot")
-	}
+	// existing holds
+	holds, err := zfs.ZFSHolds(ctx, fs, "1 2")
+	require.NoError(ctx, err)
+	require.Equal(ctx, []string{tag}, holds)
 
+	holds, err = zfs.ZFSHolds(ctx, fs, "non existent")
+	ctx.Logf("holds=%v", holds)
+	ctx.Logf("errT=%T", err)
+	ctx.Logf("err=%s", err)
+	notExist, ok := err.(*zfs.DatasetDoesNotExist)
+	require.True(ctx, ok)
+	require.Equal(ctx, fs+"@non existent", notExist.Path)
 }

platformtest/tests/listHolds.go

@@ -0,0 +1,47 @@
+package tests
+
+import (
+	"fmt"
+
+	"github.com/zrepl/zrepl/platformtest"
+	"github.com/zrepl/zrepl/zfs"
+)
+
+func ListHolds(ctx *platformtest.Context) {
+
+	platformtest.Run(ctx, platformtest.PanicErr, ctx.RootDataset, `
+		DESTROYROOT
+		CREATEROOT
+		+  "foo bar"
+		+  "foo bar@1"
+	`)
+	defer platformtest.Run(ctx, platformtest.PanicErr, ctx.RootDataset, `
+		R zfs release zrepl_platformtest "${ROOTDS}/foo bar@1"
+		-  "foo bar@1"
+		-  "foo bar"
+	`)
+
+	fs := fmt.Sprintf("%s/foo bar", ctx.RootDataset)
+	v1 := sendArgVersion(fs, "@1")
+
+	tag := "zrepl_platformtest"
+	err := zfs.ZFSHold(ctx, fs, v1, tag)
+	if err != nil {
+		panic(err)
+	}
+
+	err = zfs.ZFSHold(ctx, fs, v1, tag)
+	if err != nil {
+		panic(err)
+	}
+
+	vnonexistent := zfs.ZFSSendArgVersion{
+		RelName: "@nonexistent",
+		GUID:    0xbadf00d,
+	}
+	err = zfs.ZFSHold(ctx, fs, vnonexistent, tag)
+	if err == nil {
+		panic("still expecting error for nonexistent snapshot")
+	}
+
+}

platformtest/tests/tests.go

@@ -31,4 +31,5 @@ var Cases = []Case{
 	SendArgsValidationEncryptedSendOfUnencryptedDatasetForbidden,
 	SendArgsValidationResumeTokenEncryptionMismatchForbidden,
 	SendArgsValidationResumeTokenDifferentFilesystemForbidden,
+	ListHolds,
 }

zfs/holds.go

@@ -59,6 +59,7 @@ success:
 	return nil
 }
 
+// If the snapshot does not exist, the returned error is of type *DatasetDoesNotExist
 func ZFSHolds(ctx context.Context, fs, snap string) ([]string, error) {
 	if err := validateZFSFilesystem(fs); err != nil {
 		return nil, errors.Wrap(err, "`fs` is not a valid filesystem path")

zfs/zfs.go

@@ -350,8 +350,7 @@ func (a ZFSSendArgs) buildCommonSendArgs() ([]string, error) {
 }
 
 type ReadCloserCopier struct {
-	recorder          readErrRecorder
-	postCloseCallback func(closeErr error)
+	recorder readErrRecorder
 }
 
 type readErrRecorder struct {
@@ -403,17 +402,8 @@ func (c *ReadCloserCopier) Read(p []byte) (n int, err error) {
 	return c.recorder.Read(p)
 }
 
-// caller must ensure that this function is not executing concurrently to Close
-func (c *ReadCloserCopier) SetPostCloseCallback(callback func(closeErr error)) {
-	c.postCloseCallback = callback
-}
-
 func (c *ReadCloserCopier) Close() error {
-	err := c.recorder.Close()
-	if c.postCloseCallback != nil {
-		c.postCloseCallback(err)
-	}
-	return err
+	return c.recorder.ReadCloser.Close()
 }
 
 func pipeWithCapacityHint(capacity int) (r, w *os.File, err error) {