fix(temp_repo): cancel context on LoadDataset method

incorporating review feedback. Lots of stray comment cleanup, and a nice cancellation leak catch!

cmd/factory_test.go

@@ -2,7 +2,6 @@ package cmd
 
 import (
 	"context"
-	"fmt"
 	"net/rpc"
 	"os"
 	"path/filepath"
@@ -89,7 +88,6 @@ func NewTestFactoryInstanceOptions(ctx context.Context, opts ...lib.Option) (tf
 		lib.OptQriNode(tnode.(*p2p.QriNode)),
 	}, opts...)
 
-	fmt.Println("new test factory instance options")
 	inst, err := lib.NewInstance(ctx, "repo", opts...)
 	if err != nil {
 		return TestFactory{}, err

cmd/publish_test.go

@@ -22,7 +22,8 @@ func TestPublish(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer cleanup()
-	// TODO(b5): need to defer in this orderd. the deferred cleanup command blocks on done
+	// TODO(b5): need to defer in this order. the deferred cleanup command blocks on done,
+	// which is in turn blocked on cancel. deferring in the other order deadlocks.
 	// the smarter way to deal with this is to refactor TempRegistry to use the Done pattern
 	defer cancel()
 

cmd/qri.go

@@ -151,7 +151,6 @@ func (o *QriOptions) Init() (err error) {
 	setNoPrompt(o.NoPrompt)
 	log.Debugf("running cmd %q", os.Args)
 
-	// go o.waitForAllDone()
 	return
 }
 

lib/lib.go

@@ -372,7 +372,6 @@ func NewInstance(ctx context.Context, repoPath string, opts ...Option) (qri *Ins
 
 		if releaser, ok := inst.qfs.(qfs.ReleasingFilesystem); ok {
 			go func() {
-				//inst.waitForOneDone(releaser.Done())
 				inst.releasers.Add(1)
 				<-releaser.Done()
 				inst.doneErr = releaser.DoneErr()

registry/regserver/mock.go

@@ -48,8 +48,8 @@ func NewMemRegistry(rem *remote.Remote) registry.Registry {
 
 // NewTempRegistry creates a functioning registry with a teardown function
 // TODO(b5) - the tempRepo.Repo call in this func *requires* the passed-in
-// context be cancelled at some point. dorp the cleanup function return in
-// favour of listning for ctx.Done and running the cleanup routine internally
+// context be cancelled at some point. drop the cleanup function return in
+// favour of listening for ctx.Done and running the cleanup routine internally
 func NewTempRegistry(ctx context.Context, peername, tmpDirPrefix string, g gen.CryptoGenerator) (*registry.Registry, func(), error) {
 	tempRepo, err := repotest.NewTempRepo(peername, tmpDirPrefix, g)
 	if err != nil {

repo/test/temp_repo.go

@@ -196,7 +196,8 @@ func (r *TempRepo) DatasetMarshalJSON(ref string) (string, error) {
 
 // LoadDataset from the temp repository
 func (r *TempRepo) LoadDataset(ref string) (*dataset.Dataset, error) {
-	ctx := context.Background()
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
 	fs, err := qipfs.NewFilesystem(ctx, map[string]interface{}{
 		"online": false,
 		"path":   r.IPFSPath,
@@ -209,7 +210,10 @@ func (r *TempRepo) LoadDataset(ref string) (*dataset.Dataset, error) {
 	if err != nil {
 		return nil, err
 	}
-	return ds, nil
+	done := gracefulShutdown(fs.(qfs.ReleasingFilesystem).Done())
+	cancel()
+	err = <-done
+	return ds, err
 }
 
 // WriteRootFile writes a file string to the root directory of the temp repo