context: add refcount to allow context reuse

The PR jaypipes#236 fixes a snapshot
directory leakage issue, but also highlight a context
lifecycle/ownership issue.

In general in ghw each package creates its own context, including
the cases on which a package is consumed by another, for example
`topology` and `pci`. However, in this case, it would make more sense
to reuse the context from the parent package, because the two packages
are tightly coupled.

Before the introduction of the the transparent snapshot support
(jaypipes#202), the above mechanism worked,
because each context, each time, was consuming over and over again the
same parameters (e.g. environment variables); besides some resource
waste, this had no negative effect.

When introducing snapshots in the picture, repeatedly unpacking the
same snapshot to consume the same data is much more wasteful.
So it makes sense now to introduce explicitly the concept of dependent
context.

To move forward and make the ownership more explicit, we add an
internal reference count in context. This is used only to track the
context setup/teardown process.

When contexts are reused, we always have a parent module -which creates
and owns the context- which passes its context to a subordinate module,
because the parent wants to consume the service offered by the
subordinate. The subordinate (possibly further nested, or more than one)
should never attempt Setup() and Teardown(). These are responsabilities
of the parent, outer module, which owns the context.

It's impractical to conditionally call Setup() and Teardown(), so the
functions internally use and check the reference count to do the right
thing automatically.

The following are are idempotent (and safe and quickly return success)
- calling multiple times Setup() on a ready context
- calling multiple times Teardown() on a un-ready context

Signed-off-by: Francesco Romani <fromani@redhat.com>

pkg/context/context.go

@@ -21,6 +21,7 @@ type Context struct {
 	SnapshotExclusive    bool
 	snapshotUnpackedPath string
 	alert                option.Alerter
+	refCount             int
 }
 
 // New returns a Context struct pointer that has had various options set on it
@@ -84,8 +85,14 @@ func (ctx *Context) Do(fn func() error) error {
 // You should call `Setup` just once. It is safe to call `Setup` if you don't make
 // use of optional extra features - `Setup` will do nothing.
 func (ctx *Context) Setup() error {
+	if ctx.refCount > 1 {
+		// someone else came before and fixed things already!
+		ctx.refCount++
+		return nil
+	}
 	if ctx.SnapshotPath == "" {
-		// nothing to do!
+		// nothing to do
+		ctx.refCount++
 		return nil
 	}
 
@@ -103,26 +110,49 @@ func (ctx *Context) Setup() error {
 		}
 		_, err = snapshot.UnpackInto(ctx.SnapshotPath, root, flags)
 	}
-	if err != nil {
-		return err
+	if err == nil {
+		ctx.Chroot = root
+		ctx.refCount++
+		return nil
 	}
 
-	ctx.Chroot = root
-	return nil
+	// not ready: must try again
+	return err
 }
 
 // Teardown releases any resource acquired by Setup.
 // You should always call `Teardown` if you called `Setup` to free any resources
 // acquired by `Setup`. Check `Do` for more automated management.
 func (ctx *Context) Teardown() error {
+	if ctx.refCount > 1 {
+		// too early, the last one will do the cleaning
+		ctx.refCount--
+		return nil
+	}
 	if ctx.snapshotUnpackedPath == "" {
 		// if the client code provided the unpack directory,
 		// then it is also in charge of the cleanup.
+		ctx.refCount--
 		return nil
 	}
-	return snapshot.Cleanup(ctx.snapshotUnpackedPath)
+	err := snapshot.Cleanup(ctx.snapshotUnpackedPath)
+	if err == nil {
+		ctx.refCount--
+		return nil
+	}
+	// else need to try again later
+	return err
 }
 
 func (ctx *Context) Warn(msg string, args ...interface{}) {
 	ctx.alert.Printf("WARNING: "+msg, args...)
 }
+
+func (ctx *Context) IsReady() bool {
+	return ctx.refCount > 0
+}
+
+// RefCount() must be used only in testing code
+func (ctx *Context) RefCount() int {
+	return ctx.refCount
+}

pkg/context/context_test.go

@@ -40,3 +40,94 @@ func TestSnapshotContext(t *testing.T) {
 		t.Fatalf("Expected the uncompressed dir to be deleted: %s", uncompressedDir)
 	}
 }
+
+// nolint: gocyclo
+func TestContextReadiness(t *testing.T) {
+	ctx := context.New()
+	if ctx.IsReady() {
+		t.Fatalf("context ready before Setup()")
+	}
+
+	ctx.Do(func() error {
+		if !ctx.IsReady() {
+			t.Fatalf("context NOT ready inside Do()")
+		}
+		return nil
+	})
+
+	if ctx.IsReady() {
+		t.Fatalf("context ready after Teardown()")
+	}
+}
+
+// nolint: gocyclo
+func TestContextReadinessNested(t *testing.T) {
+	ctx := context.New()
+	if ctx.IsReady() {
+		t.Fatalf("context ready before Setup()")
+	}
+
+	ctx.Do(func() error {
+		if !ctx.IsReady() {
+			t.Fatalf("context NOT ready inside outer Do()")
+		}
+		ctx.Do(func() error {
+			if !ctx.IsReady() {
+				t.Fatalf("context NOT ready inside inner Do()")
+			}
+			return nil
+		})
+		if !ctx.IsReady() {
+			t.Fatalf("context NOT ready after inner Do()")
+		}
+		return nil
+	})
+
+	if ctx.IsReady() {
+		t.Fatalf("context ready after Teardown() - refcount = %d", ctx.RefCount())
+	}
+}
+
+// nolint: gocyclo
+func TestContextReadinessDeeplyNested(t *testing.T) {
+	// we don't expect more nesting than this atm
+	ctx := context.New()
+	if ctx.IsReady() {
+		t.Fatalf("context ready before Setup()")
+	}
+	if ctx.RefCount() != 0 {
+		t.Fatalf("context refcount unexpected value %d", ctx.RefCount())
+	}
+
+	ctx.Do(func() error {
+		if !ctx.IsReady() {
+			t.Fatalf("context NOT ready inside outer Do()")
+		}
+		ctx.Do(func() error {
+			if !ctx.IsReady() {
+				t.Fatalf("context NOT ready inside middle Do()")
+			}
+			ctx.Do(func() error {
+				if !ctx.IsReady() {
+					t.Fatalf("context NOT ready inside inner Do()")
+				}
+				return nil
+			})
+			if !ctx.IsReady() {
+				t.Fatalf("context NOT ready after inner Do()")
+			}
+			return nil
+		})
+		if !ctx.IsReady() {
+			t.Fatalf("context NOT ready after middle Do()")
+		}
+		return nil
+	})
+
+	if ctx.IsReady() {
+		t.Fatalf("context ready after Teardown() - refcount = %d", ctx.RefCount())
+	}
+	if ctx.RefCount() != 0 {
+		t.Fatalf("context refcount unexpected value %d", ctx.RefCount())
+	}
+}