Skip to content
This repository has been archived by the owner on Jun 19, 2023. It is now read-only.

feat: use error channels #62

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion block.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ package iface

import (
"context"
path "github.com/ipfs/interface-go-ipfs-core/path"
"io"

"github.com/ipfs/interface-go-ipfs-core/options"
path "github.com/ipfs/interface-go-ipfs-core/path"
)

// BlockStat contains information about a block
Expand Down
3 changes: 3 additions & 0 deletions dht.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ type DhtAPI interface {

// FindProviders finds peers in the DHT who can provide a specific value
// given a key.
//
// The returned channel will be closed when the request completes or is
// canceled.
FindProviders(context.Context, path.Path, ...options.DhtFindProvidersOption) (<-chan peer.AddrInfo, error)
Comment on lines +23 to 25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why aren't we following the pattern here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, I'm asking IPFS to find me some providers for a key. If the request is canceled, we can just say "we're done" and walk away. That is, a partial response is still a valid response.

This is different from listing files/pins because:

  1. The request can be considered "complete" even if it returns a subset of the available providers. On the other hand, if Ls returned a subset of files/pins, that would be an incomplete result.
  2. Beyond "your arguments are invalid" and "this subsystem isn't ready (not bootstrapped)", there aren't really any useful errors the router could return other than "I can't find what you're looking for".

On the other hand, this brings up a good point. IMO, the Search function should have the same interface. That is NameAPI.Search(ctx, name, opts) (<-chan path.Path, error). Thoughts?

cc @alanshaw, I think this answers your question as well.


// Provide announces to the network that you are providing given values
Expand Down
11 changes: 5 additions & 6 deletions name.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@ type IpnsEntry interface {
Value() path.Path
}

type IpnsResult struct {
path.Path
Err error
}

// NameAPI specifies the interface to IPNS.
//
// IPNS is a PKI namespace, where names are the hashes of public keys, and the
Expand All @@ -43,5 +38,9 @@ type NameAPI interface {
//
// Note: by default, all paths read from the channel are considered unsafe,
// except the latest (last path in channel read buffer).
Search(ctx context.Context, name string, opts ...options.NameResolveOption) (<-chan IpnsResult, error)
//
// Both returned channels will be closed when the request completes or is
// canceled. The error channel will yield at most one error and should
// be read after the path channel is closed.
Search(ctx context.Context, name string, opts ...options.NameResolveOption) (<-chan path.Path, <-chan error)
}
20 changes: 20 additions & 0 deletions name_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package iface

import (
"context"
"fmt"

"github.com/ipfs/interface-go-ipfs-core/path"
)

func ExampleNameAPI_Search() {
var api CoreAPI

_ = func(ctx context.Context) (result path.Path, err error) {
results, errC := api.Name().Search(ctx, "foobar")
for result = range results {
fmt.Println(result)
}
return result, <-errC
}
}
23 changes: 16 additions & 7 deletions pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ type Pin interface {

// Type of the pin
Type() string

// if not nil, an error happened. Everything else should be ignored.
Err() error
}

// PinStatus holds information about pin health
Expand All @@ -43,8 +40,12 @@ type PinAPI interface {
// tree
Add(context.Context, path.Path, ...options.PinAddOption) error

// Ls returns list of pinned objects on this node
Ls(context.Context, ...options.PinLsOption) (<-chan Pin, error)
// Ls returns list of pinned objects on this node.
//
// Both returned channels will be closed when the request completes or is
// canceled. The error channel will yield at most one error and should
// be read after the path channel is closed.
Ls(context.Context, ...options.PinLsOption) (<-chan Pin, <-chan error)

// IsPinned returns whether or not the given cid is pinned
// and an explanation of why its pinned
Expand All @@ -57,6 +58,14 @@ type PinAPI interface {
// the old tree
Update(ctx context.Context, from path.Path, to path.Path, opts ...options.PinUpdateOption) error

// Verify verifies the integrity of pinned objects
Verify(context.Context) (<-chan PinStatus, error)
// Verify verifies the integrity of pinned objects.
//
// Both returned channels will be closed when the request completes or is
// canceled. The error channel will yield at most one error and should
// be read after the path channel is closed.
//
// The error channel will only yield an error if the verification
// process itself fails or is canceled. It will not yield an error when
// we fail to verify a pin.
Verify(context.Context) (<-chan PinStatus, <-chan error)
}
40 changes: 40 additions & 0 deletions pin_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
package iface

import (
"context"
"fmt"
)

func ExamplePinAPI_Ls() {
var api CoreAPI

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

pins, errC := api.Pin().Ls(ctx)
for pin := range pins {
fmt.Println(pin.Path())
}
if err := <-errC; err != nil {
fmt.Printf("error: %s\n", err)
}
}

func ExamplePinAPI_Verify() {
var api CoreAPI

ctx, cancel := context.WithCancel(context.Background())
defer cancel()

status, errC := api.Pin().Verify(ctx)
for pin := range status {
if !pin.Ok() {
for _, missing := range pin.BadNodes() {
fmt.Printf("missing node %s: %s\n", missing.Path().Cid(), missing.Err())
}
}
}
if err := <-errC; err != nil {
fmt.Printf("failed to complete pin verification request: %s\n", err)
}
}
21 changes: 6 additions & 15 deletions tests/pin.go
Original file line number Diff line number Diff line change
Expand Up @@ -192,17 +192,17 @@ func (tp *TestSuite) TestPinRecursive(t *testing.T) {
t.Errorf("unexpected path, %s != %s", list[0].Path().Cid().String(), p0.Cid().String())
}

res, err := api.Pin().Verify(ctx)
if err != nil {
t.Fatal(err)
}
res, errCh := api.Pin().Verify(ctx)
n := 0
for r := range res {
if !r.Ok() {
t.Error("expected pin to be ok")
}
n++
}
if err := <-errCh; err != nil {
t.Fatal(err)
}

if n != 1 {
t.Errorf("unexpected verify result count: %d", n)
Expand Down Expand Up @@ -583,19 +583,10 @@ func assertNotPinned(t *testing.T, ctx context.Context, api iface.CoreAPI, p pat
}
}

func accPins(pins <-chan iface.Pin, err error) ([]iface.Pin, error) {
if err != nil {
return nil, err
}

func accPins(pins <-chan iface.Pin, err <-chan error) ([]iface.Pin, error) {
var result []iface.Pin

for pin := range pins {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, when an error happen, the result channel is not closed right ? Doesn't that means the caller would get stuck in this reading loop ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the new design, both the result and the error channels are always closed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I need to document that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the issue description and the interface documentation. It looks like docker isn't actually all that consistent.

if pin.Err() != nil {
return nil, pin.Err()
}
result = append(result, pin)
}

return result, nil
return result, <-err
}
48 changes: 26 additions & 22 deletions tests/unixfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -681,15 +681,9 @@ func (tp *TestSuite) TestLs(t *testing.T) {
t.Fatal(err)
}

entries, err := api.Unixfs().Ls(ctx, p)
if err != nil {
t.Fatal(err)
}
entries, errCh := api.Unixfs().Ls(ctx, p)

entry := <-entries
if entry.Err != nil {
t.Fatal(entry.Err)
}
if entry.Size != 15 {
t.Errorf("expected size = 15, got %d", entry.Size)
}
Expand All @@ -703,9 +697,6 @@ func (tp *TestSuite) TestLs(t *testing.T) {
t.Errorf("expected cid = QmX3qQVKxDGz3URVC3861Z3CKtQKGBn6ffXRBBWGMFz9Lr, got %s", entry.Cid)
}
entry = <-entries
if entry.Err != nil {
t.Fatal(entry.Err)
}
if entry.Type != coreiface.TSymlink {
t.Errorf("wrong type %s", entry.Type)
}
Expand All @@ -716,11 +707,11 @@ func (tp *TestSuite) TestLs(t *testing.T) {
t.Errorf("expected symlink target to be /foo/bar, got %s", entry.Target)
}

if l, ok := <-entries; ok {
if _, ok := <-entries; ok {
t.Errorf("didn't expect a second link")
if l.Err != nil {
t.Error(l.Err)
}
}
if err := <-errCh; err != nil {
t.Error(err)
}
}

Expand Down Expand Up @@ -784,14 +775,20 @@ func (tp *TestSuite) TestLsEmptyDir(t *testing.T) {
t.Fatal(err)
}

links, err := api.Unixfs().Ls(ctx, path.IpfsPath(emptyDir.Cid()))
if err != nil {
t.Fatal(err)
links, errCh := api.Unixfs().Ls(ctx, path.IpfsPath(emptyDir.Cid()))
count := 0
for range links {
count++
}

if len(links) != 0 {
if count != 0 {
t.Fatalf("expected 0 links, got %d", len(links))
}

err = <-errCh
if err != nil {
t.Fatal(err)
}
}

// TODO(lgierth) this should test properly, with len(links) > 0
Expand All @@ -813,14 +810,21 @@ func (tp *TestSuite) TestLsNonUnixfs(t *testing.T) {
t.Fatal(err)
}

links, err := api.Unixfs().Ls(ctx, path.IpfsPath(nd.Cid()))
if err != nil {
t.Fatal(err)
links, errCh := api.Unixfs().Ls(ctx, path.IpfsPath(nd.Cid()))

count := 0
for range links {
count++
}

if len(links) != 0 {
if count != 0 {
t.Fatalf("expected 0 links, got %d", len(links))
}

err = <-errCh
if err != nil {
t.Fatal(err)
}
}

type closeTestF struct {
Expand Down
4 changes: 1 addition & 3 deletions unixfs.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,6 @@ type DirEntry struct {
Size uint64 // The size of the file in bytes (or the size of the symlink).
Type FileType // The type of the file.
Target string // The symlink target (if a symlink).

Err error
}

// UnixfsAPI is the basic interface to immutable files in IPFS
Expand All @@ -75,5 +73,5 @@ type UnixfsAPI interface {

// Ls returns the list of links in a directory. Links aren't guaranteed to be
// returned in order
Ls(context.Context, path.Path, ...options.UnixfsLsOption) (<-chan DirEntry, error)
Ls(context.Context, path.Path, ...options.UnixfsLsOption) (<-chan DirEntry, <-chan error)
}
20 changes: 0 additions & 20 deletions util.go

This file was deleted.