Consecutive call to Store.Diff() never returns

[negrel]

Hello,
I've a problem using storage package, whenever I run a call to Store.Diff() followed by another call to Store.Diff(), the second call never returns. In the following code, the second print is never called.
I've also tried to debug the code using delve and it seems that store.Diff() is waiting indefinitely for the storage lockfile.

package main

import (
	"fmt"

	"github.com/containers/storage"
	"github.com/containers/storage/pkg/reexec"
	"github.com/containers/storage/pkg/unshare"
	"github.com/containers/storage/types"
)

func main() {
	if reexec.Init() {
		return
	}
	unshare.MaybeReexecUsingUserNamespace(false)

	storeOptions, err := types.DefaultStoreOptionsAutoDetectUID()
	if err != nil {
		panic(err)
	}
	store, err := storage.GetStore(storeOptions)
	if err != nil {
		panic(err)
	}

	img, err := store.Image("docker.io/library/ubuntu:22.04")
	if err != nil {
		panic(err)
	}

	diff, err := store.Diff(img.TopLayer, img.TopLayer, nil)
	if err != nil {
		panic(err)
	}
	fmt.Println("first diff done", diff)

	diff, err = store.Diff(img.TopLayer, img.TopLayer, nil)
	if err != nil {
		panic(err)
	}
	fmt.Println("second diff done", diff)
}

Here is the go.mod:

module github.com/negrel/storage-issue

go 1.18

require github.com/containers/storage v1.44.0

require (
	github.com/BurntSushi/toml v1.2.1 // indirect
	github.com/Microsoft/go-winio v0.5.2 // indirect
	github.com/Microsoft/hcsshim v0.9.5 // indirect
	github.com/containerd/cgroups v1.0.1 // indirect
	github.com/docker/go-units v0.5.0 // indirect
	github.com/gogo/protobuf v1.3.2 // indirect
	github.com/golang/groupcache v0.0.0-20200121045136-8c9f03a8e57e // indirect
	github.com/google/go-intervals v0.0.2 // indirect
	github.com/google/uuid v1.2.0 // indirect
	github.com/hashicorp/errwrap v1.0.0 // indirect
	github.com/hashicorp/go-multierror v1.1.1 // indirect
	github.com/json-iterator/go v1.1.12 // indirect
	github.com/klauspost/compress v1.15.12 // indirect
	github.com/klauspost/pgzip v1.2.5 // indirect
	github.com/mattn/go-shellwords v1.0.12 // indirect
	github.com/mistifyio/go-zfs/v3 v3.0.0 // indirect
	github.com/moby/sys/mountinfo v0.6.2 // indirect
	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
	github.com/modern-go/reflect2 v1.0.2 // indirect
	github.com/opencontainers/go-digest v1.0.0 // indirect
	github.com/opencontainers/runc v1.1.4 // indirect
	github.com/opencontainers/runtime-spec v1.0.3-0.20210326190908-1c3f411f0417 // indirect
	github.com/opencontainers/selinux v1.10.2 // indirect
	github.com/pkg/errors v0.9.1 // indirect
	github.com/sirupsen/logrus v1.9.0 // indirect
	github.com/syndtr/gocapability v0.0.0-20200815063812-42c35b437635 // indirect
	github.com/tchap/go-patricia v2.3.0+incompatible // indirect
	github.com/ulikunitz/xz v0.5.10 // indirect
	github.com/vbatts/tar-split v0.11.2 // indirect
	go.opencensus.io v0.22.3 // indirect
	golang.org/x/net v0.0.0-20210825183410-e898025ed96a // indirect
	golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8 // indirect
)

[rhatdan]

@mtrmac is this an aria where you have been lately?

[mtrmac]

Yes.

(Fundamentally the test case is… somewhat suspicious. The caller must read the generated diff to the end in order to unlock the store. That said, there are genuine problems.)

---

For some reason related to s.graphLock, store.getLayerStore() is triggering creation of a separate/refreshed store on the second Diff; and that now requires a write lock, blocking until the output of the first diff is consumed.

As suggested above, this should not be a correctness issue, because callers can make progress by consuming the first diff, but because now more strictly serialize accesses to the store in order to do cleanup, this has the effect of completely serializing Diff calls (and turning parallel pushes into sequential ones).

Note to self: We probably need to drop inProcessLock in #1346 for the same reason.

Now, if only there was any documentation for what graphLock is supposed to be protecting…

[mtrmac]

The caller must read the generated diff to the end in order to unlock the store.

Actually what matters is diff.Close().

[mtrmac]

(For the record, this was changed in #1351 , released in 1.43.0 . But the timing of this report WRT ongoing work on #1436 is really very fortunate.)