-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a 'skip' parameter to writev1 so that the beginning of a car can … #291
Open
willscott
wants to merge
11
commits into
master
Choose a base branch
from
resumecarwrite
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+883
−79
Open
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
39f18cc
Add a 'skip' parameter to writev1 so that the beginning of a car can …
willscott e7ced92
expand interface, expose reader
willscott 1a1ff47
track block offsets in traversal resumption to allow for rewinding to…
willscott 06197c5
static check
willscott 23b28c4
skip offset should be an opt
aarshkshah1992 e642855
Update options.go
aarshkshah1992 5ee6f81
more efficient + clearer teeing writer
willscott 78b25a2
test demonstrating arbitrary offsets
willscott c3789f2
rename per code review
willscott e9c7d8a
feat: make SkipWriterReaderSeeker an io.Closer
rvagg 1f6d498
fix: minor doc fixes, updates and improvements from review
rvagg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
package io | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"fmt" | ||
"io" | ||
) | ||
|
||
// SkipWriterReaderSeeker wraps a factory producing a writer with a ReadSeeker implementation. | ||
// Note that Read and Seek are not thread-safe, they must not be called | ||
// concurrently. | ||
type SkipWriterReaderSeeker struct { | ||
parentCtx context.Context | ||
offset uint64 | ||
size uint64 | ||
|
||
cons ReWriter | ||
reader *io.PipeReader | ||
writeCancel context.CancelFunc | ||
writeComplete chan struct{} | ||
} | ||
|
||
// ReWriter is a function writing to an io.Writer from an offset. | ||
type ReWriter func(ctx context.Context, skip uint64, w io.Writer) (uint64, error) | ||
|
||
var _ io.ReadSeeker = (*SkipWriterReaderSeeker)(nil) | ||
var _ io.Closer = (*SkipWriterReaderSeeker)(nil) | ||
|
||
// NewSkipWriterReaderSeeker creates an io.ReadSeeker around a ReWriter. | ||
func NewSkipWriterReaderSeeker(ctx context.Context, size uint64, cons ReWriter) *SkipWriterReaderSeeker { | ||
return &SkipWriterReaderSeeker{ | ||
parentCtx: ctx, | ||
size: size, | ||
cons: cons, | ||
writeComplete: make(chan struct{}, 1), | ||
} | ||
} | ||
|
||
// Note: not threadsafe | ||
func (c *SkipWriterReaderSeeker) Read(p []byte) (int, error) { | ||
// Check if there's already a write in progress | ||
if c.reader == nil { | ||
// No write in progress, start a new write from the current offset | ||
// in a go routine and feed it back to the caller via a pipe | ||
writeCtx, writeCancel := context.WithCancel(c.parentCtx) | ||
c.writeCancel = writeCancel | ||
pr, pw := io.Pipe() | ||
c.reader = pr | ||
|
||
go func() { | ||
amnt, err := c.cons(writeCtx, c.offset, pw) | ||
c.offset += amnt | ||
if err != nil && !errors.Is(err, context.Canceled) { | ||
pw.CloseWithError(err) | ||
} else { | ||
pw.Close() | ||
} | ||
c.writeComplete <- struct{}{} | ||
}() | ||
} | ||
|
||
return c.reader.Read(p) | ||
} | ||
|
||
// Note: not threadsafe | ||
func (c *SkipWriterReaderSeeker) Seek(offset int64, whence int) (int64, error) { | ||
// Update the offset | ||
switch whence { | ||
case io.SeekStart: | ||
if offset < 0 { | ||
return 0, fmt.Errorf("invalid offset %d from start: must be positive", offset) | ||
} | ||
c.offset = uint64(offset) | ||
case io.SeekCurrent: | ||
if int64(c.offset)+offset < 0 { | ||
return 0, fmt.Errorf("invalid offset %d from current %d: resulting offset is negative", offset, c.offset) | ||
} | ||
c.offset = uint64((int64(c.offset) + offset)) | ||
case io.SeekEnd: | ||
if c.size == 0 { | ||
return 0, ErrUnsupported | ||
|
||
} | ||
if int64(c.size)+offset < 0 { | ||
return 0, fmt.Errorf("invalid offset %d from end: larger than total size %d", offset, c.size) | ||
} | ||
c.offset = uint64(int64(c.size) + offset) | ||
} | ||
|
||
// Cancel any ongoing write and wait for it to complete | ||
// TODO: if we're fast-forwarding with 'SeekCurrent', we may be able to read from the current reader instead. | ||
if c.reader != nil { | ||
err := c.Close() | ||
c.reader = nil | ||
if err != nil { | ||
return 0, err | ||
} | ||
} | ||
|
||
return int64(c.offset), nil | ||
} | ||
|
||
func (c *SkipWriterReaderSeeker) Close() error { | ||
c.writeCancel() | ||
// Seek and Read should not be called concurrently so this is safe | ||
c.reader.Close() | ||
|
||
select { | ||
case <-c.parentCtx.Done(): | ||
return c.parentCtx.Err() | ||
case <-c.writeComplete: | ||
} | ||
return nil | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,13 +9,13 @@ import ( | |
"github.com/multiformats/go-varint" | ||
) | ||
|
||
// counter tracks how much data has been read. | ||
type counter struct { | ||
totalRead uint64 | ||
// Counter tracks how much data has been read. | ||
type Counter struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we use a struct for this? is it something to do with needing a pointer to it? why isn't a |
||
TotalRead uint64 | ||
} | ||
|
||
func (c *counter) Size() uint64 { | ||
return c.totalRead | ||
func (c *Counter) Size() uint64 { | ||
return c.TotalRead | ||
} | ||
|
||
// ReadCounter provides an externally consumable interface to the | ||
|
@@ -26,12 +26,12 @@ type ReadCounter interface { | |
|
||
type countingReader struct { | ||
r io.Reader | ||
c *counter | ||
c *Counter | ||
} | ||
|
||
func (c *countingReader) Read(p []byte) (int, error) { | ||
n, err := c.r.Read(p) | ||
c.c.totalRead += uint64(n) | ||
c.c.TotalRead += uint64(n) | ||
return n, err | ||
} | ||
|
||
|
@@ -41,7 +41,7 @@ func (c *countingReader) Read(p []byte) (int, error) { | |
// appear in a CAR file is added to the counter (included the size of the | ||
// CID and the varint length for the block data). | ||
func CountingLinkSystem(ls ipld.LinkSystem) (ipld.LinkSystem, ReadCounter) { | ||
c := counter{} | ||
c := Counter{} | ||
clc := ls | ||
clc.StorageReadOpener = func(lc linking.LinkContext, l ipld.Link) (io.Reader, error) { | ||
r, err := ls.StorageReadOpener(lc, l) | ||
|
@@ -54,7 +54,7 @@ func CountingLinkSystem(ls ipld.LinkSystem) (ipld.LinkSystem, ReadCounter) { | |
return nil, err | ||
} | ||
size := varint.ToUvarint(uint64(n) + uint64(len(l.Binary()))) | ||
c.totalRead += uint64(len(size)) + uint64(len(l.Binary())) | ||
c.TotalRead += uint64(len(size)) + uint64(len(l.Binary())) | ||
return &countingReader{buf, &c}, nil | ||
} | ||
return clc, &c | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we have a better name than
cons
? it's not obvious to me what this name is supposed to be (consume
?), it's puzzling when we get toc.cons()
and it seems like it should be more obvious what it's doing - should it be read as "start consuming now"?