From 47e41818c092d6478a2ccdb891eff4f3b3544282 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Fri, 3 Jan 2020 18:53:00 -0800 Subject: [PATCH 1/2] fix(dagreader): remove a buggy workaround for a gateway issue We had a hack that "pretended" seeking worked when it didn't as go's HTTP library uses seeks to determine the file size. However, 1. I'm changing the gateway to actually rely on seeking working as specified. 2. We don't even need this hack. The gateway performs two types of seeks (unless a range query is passed): 1. It seeks to the beginning. We can always shortcut this. 2. It seeks to the end. The gateway now has a special "lazy" seeker to avoid seeking until we actually try to _read_. Therefore, we don't need a hack for that either. --- io/dagreader.go | 20 ++++++-------- io/dagreader_test.go | 65 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 12 deletions(-) diff --git a/io/dagreader.go b/io/dagreader.go index 75fb5c714..8dcb5c911 100644 --- a/io/dagreader.go +++ b/io/dagreader.go @@ -16,6 +16,7 @@ var ( ErrIsDir = errors.New("this dag node is a directory") ErrCantReadSymlinks = errors.New("cannot currently read symlinks") ErrUnkownNodeType = errors.New("unknown node type") + ErrSeekNotSupported = errors.New("file does not support seeking") ) // TODO: Rename the `DagReader` interface, this doesn't read *any* DAG, just @@ -345,7 +346,7 @@ func (dr *dagReader) Seek(offset int64, whence int) (int64, error) { switch whence { case io.SeekStart: if offset < 0 { - return -1, errors.New("invalid offset") + return dr.offset, errors.New("invalid offset") } if offset == dr.offset { @@ -359,6 +360,11 @@ func (dr *dagReader) Seek(offset int64, whence int) (int64, error) { // Seek from the beginning of the DAG. dr.resetPosition() + // Shortcut seeking to the beginning, we're already there. + if offset == 0 { + return 0, nil + } + // Use the internal reader's context to fetch the child node promises // (see `ipld.NavigableIPLDNode.FetchChild` for details). dr.dagWalker.SetContext(dr.ctx) @@ -388,7 +394,7 @@ func (dr *dagReader) Seek(offset int64, whence int) (int64, error) { // If there aren't enough size hints don't seek // (see the `io.EOF` handling error comment below). if fsNode.NumChildren() != len(node.Links()) { - return io.EOF + return ErrSeekNotSupported } // Internal nodes have no data, so just iterate through the @@ -445,16 +451,6 @@ func (dr *dagReader) Seek(offset int64, whence int) (int64, error) { } }) - if err == io.EOF { - // TODO: Taken from https://github.com/ipfs/go-ipfs/pull/4320, - // check if still valid. - // Return negative number if we can't figure out the file size. Using io.EOF - // for this seems to be good(-enough) solution as it's only returned by - // precalcNextBuf when we step out of file range. - // This is needed for gateway to function properly - return -1, nil - } - if err != nil { return 0, err } diff --git a/io/dagreader_test.go b/io/dagreader_test.go index 884ae332d..de664370c 100644 --- a/io/dagreader_test.go +++ b/io/dagreader_test.go @@ -72,6 +72,71 @@ func TestSeekAndRead(t *testing.T) { } } +func TestSeekWithoutBlocksizes(t *testing.T) { + dserv := testu.GetDAGServ() + ctx, closer := context.WithCancel(context.Background()) + defer closer() + + inbuf := make([]byte, 1024) + + for i := 0; i < 256; i++ { + inbuf[i*4] = byte(i) + } + + inbuf[1023] = 1 // force the reader to be 1024 bytes + node := testu.GetNode(t, dserv, inbuf, testu.UseProtoBufLeaves) + + // remove the blocksizes + pbnode := node.Copy().(*mdag.ProtoNode) + fsnode, err := unixfs.FSNodeFromBytes(pbnode.Data()) + if err != nil { + t.Fatal(err) + } + fsnode.RemoveAllBlockSizes() + newData, err := fsnode.GetBytes() + if err != nil { + t.Fatal(err) + } + pbnode.SetData(newData) + err = dserv.Add(ctx, pbnode) + if err != nil { + t.Fatal(err) + } + node = pbnode + + reader, err := NewDagReader(ctx, node, dserv) + if err != nil { + t.Fatal(err) + } + + _, err = reader.Seek(-4, io.SeekEnd) + if err == nil { + t.Fatal("seeking shouldn't work without blocksizes") + } + + _, err = reader.Seek(4, io.SeekStart) + if err == nil { + t.Fatal("seeking shouldn't work without blocksizes") + } + + _, err = reader.Seek(4, io.SeekCurrent) + if err == nil { + t.Fatal("seeking shouldn't work without blocksizes") + } + + // Seeking to the current position or the end should still work. + + _, err = reader.Seek(0, io.SeekCurrent) + if err != nil { + t.Fatal(err) + } + + _, err = reader.Seek(0, io.SeekStart) + if err != nil { + t.Fatal(err) + } +} + func TestRelativeSeek(t *testing.T) { dserv := testu.GetDAGServ() ctx, closer := context.WithCancel(context.Background()) From d1f8577538c558e0dd5abc826a97bdfe715e84c9 Mon Sep 17 00:00:00 2001 From: Steven Allen Date: Sun, 5 Jan 2020 16:32:32 -0800 Subject: [PATCH 2/2] fix(io): make resetPosition actually reset everything Including the offset. --- io/dagreader.go | 1 + 1 file changed, 1 insertion(+) diff --git a/io/dagreader.go b/io/dagreader.go index 8dcb5c911..374b50916 100644 --- a/io/dagreader.go +++ b/io/dagreader.go @@ -480,6 +480,7 @@ func (dr *dagReader) Seek(offset int64, whence int) (int64, error) { // in the `SeekStart` case. func (dr *dagReader) resetPosition() { dr.currentNodeData = nil + dr.offset = 0 dr.dagWalker = ipld.NewWalker(dr.ctx, ipld.NewNavigableIPLDNode(dr.rootNode, dr.serv)) // TODO: This could be avoided (along with storing the `dr.rootNode` and