From 3b0d257e2a6c666c1fc75329610f0256652f3cbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Mur=C3=A9?= Date: Mon, 16 Jan 2023 17:19:27 +0100 Subject: [PATCH 1/2] add and connect missing go context --- dspinner/pin.go | 7 ++----- dspinner/pin_test.go | 4 ++-- pin.go | 4 ++-- 3 files changed, 6 insertions(+), 9 deletions(-) diff --git a/dspinner/pin.go b/dspinner/pin.go index fa3d9e7..a9de136 100644 --- a/dspinner/pin.go +++ b/dspinner/pin.go @@ -558,8 +558,7 @@ func (p *pinner) CheckIfPinned(ctx context.Context, cids ...cid.Cid) ([]ipfspinn // RemovePinWithMode is for manually editing the pin structure. // Use with care! If used improperly, garbage collection may not // be successful. -func (p *pinner) RemovePinWithMode(c cid.Cid, mode ipfspinner.Mode) { - ctx := context.TODO() +func (p *pinner) RemovePinWithMode(ctx context.Context, c cid.Cid, mode ipfspinner.Mode) { // Check cache to see if CID is pinned switch mode { case ipfspinner.Direct, ipfspinner.Recursive: @@ -826,9 +825,7 @@ func (p *pinner) Flush(ctx context.Context) error { // PinWithMode allows the user to have fine grained control over pin // counts -func (p *pinner) PinWithMode(c cid.Cid, mode ipfspinner.Mode) { - ctx := context.TODO() - +func (p *pinner) PinWithMode(ctx context.Context, c cid.Cid, mode ipfspinner.Mode) { p.lock.Lock() defer p.lock.Unlock() diff --git a/dspinner/pin_test.go b/dspinner/pin_test.go index 4e12fef..25303e9 100644 --- a/dspinner/pin_test.go +++ b/dspinner/pin_test.go @@ -409,7 +409,7 @@ func TestRemovePinWithMode(t *testing.T) { t.Error("pin should not have been removed") } - p.RemovePinWithMode(ak, ipfspin.Direct) + p.RemovePinWithMode(ctx, ak, ipfspin.Direct) assertUnpinned(t, p, ak, "pin was not removed") } @@ -523,7 +523,7 @@ func TestFlush(t *testing.T) { } _, k := randNode() - p.PinWithMode(k, ipfspin.Recursive) + p.PinWithMode(ctx, k, ipfspin.Recursive) if err = p.Flush(ctx); err != nil { t.Fatal(err) } diff --git a/pin.go b/pin.go index bbabac5..7d224ee 100644 --- a/pin.go +++ b/pin.go @@ -111,12 +111,12 @@ type Pinner interface { // PinWithMode is for manually editing the pin structure. Use with // care! If used improperly, garbage collection may not be // successful. - PinWithMode(cid.Cid, Mode) + PinWithMode(context.Context, cid.Cid, Mode) // RemovePinWithMode is for manually editing the pin structure. // Use with care! If used improperly, garbage collection may not // be successful. - RemovePinWithMode(cid.Cid, Mode) + RemovePinWithMode(context.Context, cid.Cid, Mode) // Flush writes the pin state to the backing datastore Flush(ctx context.Context) error From 369f7cb292574535df7d009ab4400579883da7c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Mur=C3=A9?= Date: Tue, 17 Jan 2023 13:40:12 +0100 Subject: [PATCH 2/2] Make PinWithMode() return an error, share most of the code with Pin() Now that PinWithMode() takes a context and return an error, there is not that much difference with Pin(). Those differences are: - Pin() store the root node and fetch the graph if recursive - Pin() accept an ipld.Node, PinWithMode() accept a CID (kinda meaningless) - Pin() accept a "recurse" boolean, PinWithMode() accept a Mode, but only Direct and Recursive values There is a case to be made to merge those or eliminate one. --- dspinner/pin.go | 162 ++++++++++++++++++------------------------- dspinner/pin_test.go | 42 +---------- pin.go | 9 +-- 3 files changed, 73 insertions(+), 140 deletions(-) diff --git a/dspinner/pin.go b/dspinner/pin.go index a9de136..efe36df 100644 --- a/dspinner/pin.go +++ b/dspinner/pin.go @@ -13,14 +13,15 @@ import ( "github.com/ipfs/go-cid" ds "github.com/ipfs/go-datastore" "github.com/ipfs/go-datastore/query" - ipfspinner "github.com/ipfs/go-ipfs-pinner" - "github.com/ipfs/go-ipfs-pinner/dsindex" ipld "github.com/ipfs/go-ipld-format" logging "github.com/ipfs/go-log" "github.com/ipfs/go-merkledag" "github.com/ipfs/go-merkledag/dagutils" "github.com/polydawn/refmt/cbor" "github.com/polydawn/refmt/obj/atlas" + + ipfspinner "github.com/ipfs/go-ipfs-pinner" + "github.com/ipfs/go-ipfs-pinner/dsindex" ) const ( @@ -179,23 +180,30 @@ func (p *pinner) Pin(ctx context.Context, node ipld.Node, recurse bool) error { return err } - c := node.Cid() + if recurse { + return p.doPinRecursive(ctx, node.Cid(), true) + } else { + return p.doPinDirect(ctx, node.Cid()) + } +} + +func (p *pinner) doPinRecursive(ctx context.Context, c cid.Cid, fetch bool) error { cidKey := c.KeyString() p.lock.Lock() defer p.lock.Unlock() - if recurse { - found, err := p.cidRIndex.HasAny(ctx, cidKey) - if err != nil { - return err - } - if found { - return nil - } + found, err := p.cidRIndex.HasAny(ctx, cidKey) + if err != nil { + return err + } + if found { + return nil + } - dirtyBefore := p.dirty + dirtyBefore := p.dirty + if fetch { // temporary unlock to fetch the entire graph p.lock.Unlock() // Fetch graph starting at node identified by cid @@ -204,57 +212,66 @@ func (p *pinner) Pin(ctx context.Context, node ipld.Node, recurse bool) error { if err != nil { return err } + } - // If autosyncing, sync dag service before making any change to pins - err = p.flushDagService(ctx, false) - if err != nil { - return err - } - - // Only look again if something has changed. - if p.dirty != dirtyBefore { - found, err = p.cidRIndex.HasAny(ctx, cidKey) - if err != nil { - return err - } - if found { - return nil - } - } + // If autosyncing, sync dag service before making any change to pins + err = p.flushDagService(ctx, false) + if err != nil { + return err + } - // TODO: remove this to support multiple pins per CID - found, err = p.cidDIndex.HasAny(ctx, cidKey) + // Only look again if something has changed. + if p.dirty != dirtyBefore { + found, err = p.cidRIndex.HasAny(ctx, cidKey) if err != nil { return err } if found { - _, err = p.removePinsForCid(ctx, c, ipfspinner.Direct) - if err != nil { - return err - } + return nil } + } - _, err = p.addPin(ctx, c, ipfspinner.Recursive, "") - if err != nil { - return err - } - } else { - found, err := p.cidRIndex.HasAny(ctx, cidKey) + // TODO: remove this to support multiple pins per CID + found, err = p.cidDIndex.HasAny(ctx, cidKey) + if err != nil { + return err + } + if found { + _, err = p.removePinsForCid(ctx, c, ipfspinner.Direct) if err != nil { return err } - if found { - return fmt.Errorf("%s already pinned recursively", c.String()) - } + } - _, err = p.addPin(ctx, c, ipfspinner.Direct, "") - if err != nil { - return err - } + _, err = p.addPin(ctx, c, ipfspinner.Recursive, "") + if err != nil { + return err } return p.flushPins(ctx, false) } +func (p *pinner) doPinDirect(ctx context.Context, c cid.Cid) error { + cidKey := c.KeyString() + + p.lock.Lock() + defer p.lock.Unlock() + + found, err := p.cidRIndex.HasAny(ctx, cidKey) + if err != nil { + return err + } + if found { + return fmt.Errorf("%s already pinned recursively", c.String()) + } + + _, err = p.addPin(ctx, c, ipfspinner.Direct, "") + if err != nil { + return err + } + + return p.flushPins(ctx, false) +} + func (p *pinner) addPin(ctx context.Context, c cid.Cid, mode ipfspinner.Mode, name string) (string, error) { // Create new pin and store in datastore pp := newPin(c, mode, name) @@ -555,34 +572,6 @@ func (p *pinner) CheckIfPinned(ctx context.Context, cids ...cid.Cid) ([]ipfspinn return pinned, nil } -// RemovePinWithMode is for manually editing the pin structure. -// Use with care! If used improperly, garbage collection may not -// be successful. -func (p *pinner) RemovePinWithMode(ctx context.Context, c cid.Cid, mode ipfspinner.Mode) { - // Check cache to see if CID is pinned - switch mode { - case ipfspinner.Direct, ipfspinner.Recursive: - default: - // programmer error, panic OK - panic("unrecognized pin type") - } - - p.lock.Lock() - defer p.lock.Unlock() - - removed, err := p.removePinsForCid(ctx, c, mode) - if err != nil { - log.Error("cound not remove pins: %s", err) - return - } - if !removed { - return - } - if err = p.flushPins(ctx, false); err != nil { - log.Error("cound not remove pins: %s", err) - } -} - // removePinsForCid removes all pins for a cid that has the specified mode. // Returns true if any pins, and all corresponding CID index entries, were // removed. Otherwise, returns false. @@ -825,30 +814,15 @@ func (p *pinner) Flush(ctx context.Context) error { // PinWithMode allows the user to have fine grained control over pin // counts -func (p *pinner) PinWithMode(ctx context.Context, c cid.Cid, mode ipfspinner.Mode) { - p.lock.Lock() - defer p.lock.Unlock() - +func (p *pinner) PinWithMode(ctx context.Context, c cid.Cid, mode ipfspinner.Mode) error { // TODO: remove his to support multiple pins per CID switch mode { case ipfspinner.Recursive: - if has, _ := p.cidRIndex.HasAny(ctx, c.KeyString()); has { - return // already a recursive pin for this CID - } + return p.doPinRecursive(ctx, c, false) case ipfspinner.Direct: - if has, _ := p.cidDIndex.HasAny(ctx, c.KeyString()); has { - return // already a direct pin for this CID - } + return p.doPinDirect(ctx, c) default: - panic("unrecognized pin mode") - } - - _, err := p.addPin(ctx, c, mode, "") - if err != nil { - return - } - if err = p.flushPins(ctx, false); err != nil { - log.Errorf("failed to create %s pin: %s", mode, err) + return fmt.Errorf("unrecognized pin mode") } } diff --git a/dspinner/pin_test.go b/dspinner/pin_test.go index 25303e9..11c7ade 100644 --- a/dspinner/pin_test.go +++ b/dspinner/pin_test.go @@ -19,10 +19,11 @@ import ( lds "github.com/ipfs/go-ds-leveldb" blockstore "github.com/ipfs/go-ipfs-blockstore" offline "github.com/ipfs/go-ipfs-exchange-offline" - ipfspin "github.com/ipfs/go-ipfs-pinner" util "github.com/ipfs/go-ipfs-util" ipld "github.com/ipfs/go-ipld-format" logging "github.com/ipfs/go-log" + + ipfspin "github.com/ipfs/go-ipfs-pinner" ) var rand = util.NewTimeSeededRand() @@ -375,45 +376,6 @@ func TestAddLoadPin(t *testing.T) { } } -func TestRemovePinWithMode(t *testing.T) { - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - dstore := dssync.MutexWrap(ds.NewMapDatastore()) - bstore := blockstore.NewBlockstore(dstore) - bserv := bs.New(bstore, offline.Exchange(bstore)) - - dserv := mdag.NewDAGService(bserv) - - p, err := New(ctx, dstore, dserv) - if err != nil { - t.Fatal(err) - } - - a, ak := randNode() - err = dserv.Add(ctx, a) - if err != nil { - panic(err) - } - - err = p.Pin(ctx, a, false) - if err != nil { - t.Fatal(err) - } - - ok, err := p.removePinsForCid(ctx, ak, ipfspin.Recursive) - if err != nil { - t.Fatal(err) - } - if ok { - t.Error("pin should not have been removed") - } - - p.RemovePinWithMode(ctx, ak, ipfspin.Direct) - - assertUnpinned(t, p, ak, "pin was not removed") -} - func TestIsPinnedLookup(t *testing.T) { // Test that lookups work in pins which share // the same branches. For that construct this tree: diff --git a/pin.go b/pin.go index 7d224ee..27f4b40 100644 --- a/pin.go +++ b/pin.go @@ -92,6 +92,8 @@ type Pinner interface { IsPinnedWithType(ctx context.Context, c cid.Cid, mode Mode) (string, bool, error) // Pin the given node, optionally recursively. + // Pin will make sure that the given node and its children if recursive is set + // are stored locally. Pin(ctx context.Context, node ipld.Node, recursive bool) error // Unpin the given cid. If recursive is true, removes either a recursive or @@ -111,12 +113,7 @@ type Pinner interface { // PinWithMode is for manually editing the pin structure. Use with // care! If used improperly, garbage collection may not be // successful. - PinWithMode(context.Context, cid.Cid, Mode) - - // RemovePinWithMode is for manually editing the pin structure. - // Use with care! If used improperly, garbage collection may not - // be successful. - RemovePinWithMode(context.Context, cid.Cid, Mode) + PinWithMode(context.Context, cid.Cid, Mode) error // Flush writes the pin state to the backing datastore Flush(ctx context.Context) error