Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Add DefaultShell as required by #69 #106

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Changes from 2 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
96 changes: 96 additions & 0 deletions shell.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"os"
"path"
"strings"
"sync"
"time"

files "github.com/ipfs/go-ipfs-cmdkit/files"
Expand All @@ -27,6 +28,13 @@ const (
DefaultPathRoot = "~/" + DefaultPathName
DefaultApiFile = "api"
EnvDir = "IPFS_PATH"
DefaultGateway = "https://ipfs.io"
DefaultAPIAddr = "/ip4/127.0.0.1/tcp/5001"
)

var (
defaultShellAddr string
defaultShellAddrLoadOnce sync.Once
)

type Shell struct {
Expand Down Expand Up @@ -84,6 +92,94 @@ func NewShellWithClient(url string, c *gohttp.Client) *Shell {
}
}

// Load all shell urls with error checking
func NewShellWithCheck(urls ...string) (*Shell, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe TryNewShell? Not happy with the name but can't think of a better one.

Anyways, this should probably have some more documentation (the description doesn't explain what "error checking" means and would otherwise lead me to believe that it returns a slice of shells).

encountered := map[string]struct{}{}
for _, url := range urls {
if _, ok := encountered[url]; !ok {
encountered[url] = struct{}{}
sh := NewShell(url)
_, _, err := sh.Version()
if err == nil {
return sh, nil
}
}
}
return nil, errors.New(fmt.Sprintf("No working server in %v", urls))
Copy link
Member

Choose a reason for hiding this comment

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

I'd just return nil instead of an error (the error isn't very useful).

}

func NewShellFromDefaultGateway() (*Shell, error) {
return NewShellWithCheck(DefaultGateway)
}

// Get all shell urls from api files
func newShellFromAPIFiles(files ...string) (urls []string) {
for _, apifile := range files {
if data, err := ioutil.ReadFile(apifile); err == nil {
url := strings.Trim(string(data), "\n\t ")
urls = append(urls, url)
}
}
return
}

func NewShellFromAPIFiles(files ...string) (*Shell, error) {
return NewShellWithCheck(newShellFromAPIFiles(files...)...)
}

// Get api url from ~/.ipfs/api
func newShellFromDefaultAPIFile() (urls []string) {
apifile, err := homedir.Expand(path.Join(DefaultPathRoot, DefaultApiFile))
if err != nil {
return urls
}
return newShellFromAPIFiles(apifile)
}

func NewShellFromDefaultAPIFile() (*Shell, error) {
return NewShellWithCheck(newShellFromDefaultAPIFile()...)
}

// get all shell urls from environmental variable IPFS_API and IPFS_PATH
func newShellFromEnv() (urls []string) {
if ipfsAPI := os.Getenv("IPFS_API"); ipfsAPI != "" {
urls = append(urls, ipfsAPI)
}
if ipfsPath := os.Getenv("IPFS_PATH"); ipfsPath != "" {
apifile := path.Join(ipfsPath, DefaultApiFile)
urls = append(urls, newShellFromAPIFiles(apifile)...)
}
return
}

func NewShellFromEnv() (*Shell, error) {
return NewShellWithCheck(newShellFromEnv()...)
}

// Load working default shell address once
func defaultShell() {
fmt.Println("ran defaultShell")
var urls []string
urls = append(urls, newShellFromEnv()...)
urls = append(urls, newShellFromDefaultAPIFile()...)
urls = append(urls, DefaultAPIAddr, DefaultGateway)
if sh, err := NewShellWithCheck(urls...); err == nil {
defaultShellAddr = sh.url
}
}

func DefaultShell() *Shell {
// Load once and cache working default shell address
defaultShellAddrLoadOnce.Do(defaultShell)
return NewShell(defaultShellAddr)
Copy link
Member

Choose a reason for hiding this comment

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

I'd just reuse the same shell. Its thread-safe and, really, we want to reuse the http client.

Also, that'd get rid of the need for DefaultShellWithCheck.

Copy link
Author

@contrun contrun Jul 17, 2018

Choose a reason for hiding this comment

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

If the apifile does exist, but an error occurred while reading the apifile, should we just return nil? This will also be surprising, as people will see unexpected panic. In this extreme case, should we just call panic?

Copy link
Member

Choose a reason for hiding this comment

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

Hm. Yeah, that's unfortunate. We shouldn't panic. We could:

  1. Return nil.
  2. Return an error (stashing it somewhere). That makes the API a bit more difficult to use but that's probably not that much of an issue.
  3. Return a special shell that always fails? Probably a bad idea.

I'd prefer 2.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mind clarify where should I put the error?

Copy link
Member

Choose a reason for hiding this comment

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

Put it in a variable, guarded by a sync.Once.

Copy link
Author

Choose a reason for hiding this comment

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

I did not add a sync.Once to reading api file. For the time being, reading api file will only be called by NewLocalShell, which is already guarded by a sync.Once. Sorry for the delay. I had a really busy week on some earthly bussiness.

Copy link
Member

Choose a reason for hiding this comment

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

No need to apologize for delays, there's no rush on this.

}

func DefaultShellWithCheck() (*Shell, error) {
// Load once and cache working default shell address
defaultShellAddrLoadOnce.Do(defaultShell)
return NewShellWithCheck(defaultShellAddr)
}

func (s *Shell) SetTimeout(d time.Duration) {
s.httpcli.Timeout = d
}
Expand Down