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

Do not store context in structs, and other fixes #84

Closed
wants to merge 2 commits into from
Closed

Do not store context in structs, and other fixes #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7533af9
Do not store context in structs
gammazero Feb 5, 2021
eb3b901
Clear dir entries cache after sync()
gammazero Feb 18, 2021
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
32 changes: 15 additions & 17 deletions dir.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,15 +27,13 @@ var ErrDirExists = errors.New("directory already has entry by that name")
type Directory struct {
inode

// Internal cache with added entries to the directory, its cotents
// Internal cache with added entries to the directory, its contents
// are synched with the underlying `unixfsDir` node in `sync()`.
entriesCache map[string]FSNode

lock sync.Mutex
// TODO: What content is being protected here exactly? The entire directory?

ctx context.Context

// UnixFS directory implementation used for creating,
// reading and editing directories.
unixfsDir uio.Directory
Expand All @@ -47,7 +45,7 @@ type Directory struct {
//
// You probably don't want to call this directly. Instead, construct a new root
// using NewRoot.
func NewDirectory(ctx context.Context, name string, node ipld.Node, parent parent, dserv ipld.DAGService) (*Directory, error) {
func NewDirectory(name string, node ipld.Node, parent parent, dserv ipld.DAGService) (*Directory, error) {
db, err := uio.NewDirectoryFromNode(dserv, node)
if err != nil {
return nil, err
Expand All @@ -59,7 +57,6 @@ func NewDirectory(ctx context.Context, name string, node ipld.Node, parent paren
parent: parent,
dagService: dserv,
},
ctx: ctx,
unixfsDir: db,
entriesCache: make(map[string]FSNode),
modTime: time.Now(),
Expand Down Expand Up @@ -117,13 +114,13 @@ func (d *Directory) localUpdate(c child) (*dag.ProtoNode, error) {
return nil, dag.ErrNotProtobuf
}

err = d.dagService.Add(d.ctx, nd)
Copy link

Choose a reason for hiding this comment

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

is it worth having context still an argument to constructor and threaded through to things like this, but not saved in the struct?

Copy link
Author

@gammazero gammazero Feb 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I have been considering this, and where the use of context was removed and replaced with context.Background() are the places that I thought it was better to not support cancellation at all.

Suppose there is a single request that creates a new directory, puts some data into that directory, deletes the data from some other place. Do we really want cancellation in the middle of this? If a request is canceled or timed out due to a stall in a lower level (e.g. datastore), it may better to let the processing continue and either eventually succeed or error out. APIs like Close and Flush probably should not support cancellation at all. Thoughts?

Contexts spread like viruses, so I was hoping to limit the spread to where absolutely necessary.

@Stebalien Do you see there is a need to thread contexts through the APIs? If so I will make that change.

Copy link
Member

Choose a reason for hiding this comment

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

This should take a context (from Flush). Flush should support cancellation as it might need to write a lot of data (and that update may need to be canceled).

err = d.dagService.Add(context.Background(), nd)
if err != nil {
return nil, err
}

// A copy is needed here since nd is referenced by dagService.
return pbnd.Copy().(*dag.ProtoNode), nil
// TODO: Why do we need a copy?
}

// Update child entry in the underlying UnixFS directory.
Expand Down Expand Up @@ -164,7 +161,7 @@ func (d *Directory) cacheNode(name string, nd ipld.Node) (FSNode, error) {

switch fsn.Type() {
case ft.TDirectory, ft.THAMTShard:
ndir, err := NewDirectory(d.ctx, name, nd, d, d.dagService)
ndir, err := NewDirectory(name, nd, d, d.dagService)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -211,7 +208,7 @@ func (d *Directory) Uncache(name string) {
// childFromDag searches through this directories dag node for a child link
// with the given name
func (d *Directory) childFromDag(name string) (ipld.Node, error) {
return d.unixfsDir.Find(d.ctx, name)
return d.unixfsDir.Find(context.Background(), name)
Copy link
Member

Choose a reason for hiding this comment

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

Yeah, in general, never use context.Background() unless something really shouldn't be cancelable. This here will make canceling reads impossible.

}

// childUnsync returns the child under this directory by the given name
Expand Down Expand Up @@ -308,7 +305,7 @@ func (d *Directory) Mkdir(name string) (*Directory, error) {
ndir := ft.EmptyDirNode()
ndir.SetCidBuilder(d.GetCidBuilder())

err = d.dagService.Add(d.ctx, ndir)
err = d.dagService.Add(context.Background(), ndir)
if err != nil {
return nil, err
}
Expand All @@ -318,7 +315,7 @@ func (d *Directory) Mkdir(name string) (*Directory, error) {
return nil, err
}

dirobj, err := NewDirectory(d.ctx, name, ndir, d, d.dagService)
dirobj, err := NewDirectory(name, ndir, d, d.dagService)
if err != nil {
return nil, err
}
Expand All @@ -333,7 +330,7 @@ func (d *Directory) Unlink(name string) error {

delete(d.entriesCache, name)

return d.unixfsDir.RemoveChild(d.ctx, name)
return d.unixfsDir.RemoveChild(context.Background(), name)
}

func (d *Directory) Flush() error {
Expand All @@ -355,7 +352,7 @@ func (d *Directory) AddChild(name string, nd ipld.Node) error {
return ErrDirExists
}

err = d.dagService.Add(d.ctx, nd)
err = d.dagService.Add(context.Background(), nd)
if err != nil {
return err
}
Expand All @@ -376,15 +373,15 @@ func (d *Directory) addUnixFSChild(c child) error {
// If the directory HAMT implementation is being used and this
// directory is actually a basic implementation switch it to HAMT.
if basicDir, ok := d.unixfsDir.(*uio.BasicDirectory); ok {
hamtDir, err := basicDir.SwitchToSharding(d.ctx)
hamtDir, err := basicDir.SwitchToSharding(context.Background())
if err != nil {
return err
}
d.unixfsDir = hamtDir
}
}

err := d.unixfsDir.AddChild(d.ctx, c.Name, c.Node)
err := d.unixfsDir.AddChild(context.Background(), c.Name, c.Node)
if err != nil {
return err
}
Expand All @@ -405,7 +402,8 @@ func (d *Directory) sync() error {
}
}

// TODO: Should we clean the cache here?
// Clear entries cache
d.entriesCache = make(map[string]FSNode)

return nil
}
Expand Down Expand Up @@ -441,7 +439,7 @@ func (d *Directory) GetNode() (ipld.Node, error) {
return nil, err
}

err = d.dagService.Add(d.ctx, nd)
err = d.dagService.Add(context.Background(), nd)
if err != nil {
return nil, err
}
Expand Down
5 changes: 4 additions & 1 deletion fd.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package mfs

import (
"errors"
"fmt"
"io"

Expand All @@ -20,6 +21,8 @@ const (
stateClosed
)

var ErrClosed = errors.New("file closed")

// One `File` can have many `FileDescriptor`s associated to it
// (only one if it's RW, many if they are RO, see `File.desclock`).
// A `FileDescriptor` contains the "view" of the file (through an
Expand Down Expand Up @@ -145,7 +148,7 @@ func (fi *fileDescriptor) flushUp(fullSync bool) error {
if err != nil {
return err
}
err = fi.inode.dagService.Add(context.TODO(), nd)
err = fi.inode.dagService.Add(context.Background(), nd)
if err != nil {
return err
}
Expand Down
6 changes: 3 additions & 3 deletions file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,9 +160,9 @@ func (fi *File) Flush() error {
return err
}

defer fd.Close()

return fd.Flush()
// Close calls fd.Flush() and will do a full sync since it was opened with
// Sync=true
return fd.Close()
}

func (fi *File) Sync() error {
Expand Down
3 changes: 1 addition & 2 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,10 @@ require (
github.com/ipfs/go-ipfs-exchange-offline v0.0.1
github.com/ipfs/go-ipfs-util v0.0.1
github.com/ipfs/go-ipld-format v0.0.2
github.com/ipfs/go-log v0.0.1
github.com/ipfs/go-merkledag v0.1.0
github.com/ipfs/go-path v0.0.7
github.com/ipfs/go-unixfs v0.1.0
github.com/libp2p/go-libp2p-testing v0.0.4
)

go 1.13
go 1.14
Loading