feat(interface) ThreadSafeDatastoreCloser

[btc]

Expose Close() to make it possible to clean up LevelDB instances.

License: MIT
Signed-off-by: Brian Tiger Chow brian@perfmode.com

[jbenet]

I want to keep the Closer interface outside of the Datastore* interfaces. I think the leveldb instance (or whatever you want the Close func in) can just conform to io.Closer.

[btc]

LevelDB returns a ThreadSafeDatastore. Prefer to expose the concrete struct?

[jbenet]

@maybebtc i think the Closer interface doesnt belong in the datastore interfaces themselves. I think the concrete types (e.g. leveldb.Datastore) can just implement Closer where needed.

[btc]

I'll have the LevelDB constructor return a concrete instance then.

[jbenet]

@maybebtc yeah that makes sense. feel free to break up leveldb.Datastore into two parts: an interface, and an unexported concrete type

[jbenet]

// Datastore wraps a levelDB instance with a Datastore interface
type Datastore interface {
    ds.ThreadSafeDatastore
    io.Closer

    DB() *leveldb.DB
}

// datastore uses a standard Go map for internal storage.
type datastore struct {
    db *leveldb.DB
}

[btc]

closed in favor of #10

[btc]

@jbenet

One minor note...

Since the datastore interfaces to not expose Close(), the client is left to implement her own wrapper. This is perfectly fine, but it might be nice for the library to anticipate this need and provide this behavior just as it does with the mutex wrapper.

Example:

// ClientAppStore returns a Datastore that may need to be closed in Production
func ClientAppStore(in_test_env bool) MyThreadSafeDatastoreCloser {
  if in_test_env {
    return ds.NewMapDatastore() // compiler error
  }
  return leveldb.NewDatastore()
}

To fix this, each go-datastore client must implement a trivial Wrapper around datastores.

type ThreadSafeDatastoreCloserWrapper struct {
  ds.ThreadSafeDatastore
}

func (w *ThreadSafeDatastoreCloserWrapper) Close() error {
return nil // no-op
}

For now, I'll implement a wrapper for my specific use case.

[jbenet]

@maybebtc i hear you. It's a tough call. A few things to bear in mind:

1. I'm worried by interface composition explosion. Every datastore sub-interface composing with all others would quickly yield rote work in writing out permutations, and awful ThreadSafeCloserNonBlockingSymlinkDatastore javarrhea. I dont see this as a viable route.

2. Close is a common enough thing that it may be ok to expand the datastore interface with. So if we reaaaally want it, i'd rather implement explicit nil Close funcs than permuations.

3. the ThreadSafeDatastore is a different case, in that there is a wrapper that implements more functionality around non-ThreadSafeDatastores, and there is an interface datastores can signal when they implement it internally. (perhaps i shouldn't have put it inside the main Datastore lib, but inside the datastore/sync pkg...)

4. This is not as nice in languages with inheritance, but in Go, the user maybe should be specifying the interface of her own datastores explicitly. I think it's easier to pick and choose what you want all your datastores to conform to:

   // in go-ipfs/core/datastore.go
   
   // Datastore is the common interface all our datastore backends must conform to
   type Datastore interface {
     ds.Datastore
     syncds.ThreadSafeDatastore
     ktds.KeyTransformDatastore
     symds.SymlinkDatastore
     io.Closer
     sync.Locker
     log.Loggable
   }

So i think for now let's keep as is? We can revisit this if a better strategy is found.

[btc]

I agree with all points. There's no harm in being careful about over-extending the library.

as an aside, here's a link to the wrapper: https://github.com/jbenet/go-ipfs/blob/feat/daemon-init/util/datastore_closer.go