diff --git a/pkg/archive/archive_exploded.go b/pkg/archive/archive_exploded.go index 55c03e66..7efe942c 100644 --- a/pkg/archive/archive_exploded.go +++ b/pkg/archive/archive_exploded.go @@ -31,13 +31,17 @@ func (e explodedArchiveEntry) CRC32Checksum() *uint32 { } func (e explodedArchiveEntry) CompressedAs(compressionMethod CompressionMethod) bool { - return false + // Exploded-archive entries are plain files, i.e. stored uncompressed. + return compressionMethod == CompressionMethodStore } func (e explodedArchiveEntry) Read(start int64, end int64) ([]byte, error) { if end < start { return nil, errors.New("range not satisfiable") } + if start < 0 { + start = 0 + } f, err := os.Open(filepath.Join(e.dir, e.filepath)) if err != nil { return nil, err @@ -70,6 +74,9 @@ func (e explodedArchiveEntry) Stream(w io.Writer, start int64, end int64) (int64 if end < start { return -1, errors.New("range not satisfiable") } + if start < 0 { + start = 0 + } f, err := os.Open(filepath.Join(e.dir, e.filepath)) if err != nil { return -1, err diff --git a/pkg/archive/archive_zip.go b/pkg/archive/archive_zip.go index 7462d441..67acf9ee 100644 --- a/pkg/archive/archive_zip.go +++ b/pkg/archive/archive_zip.go @@ -8,6 +8,7 @@ import ( "io" "io/fs" "math" + "os" "path" "sync" @@ -19,10 +20,51 @@ type gozipArchiveEntry struct { file *zip.File minimizeReads bool + // path is the filesystem path of the archive when it was opened from a + // local file, or empty otherwise. It unlocks streaming an entry's raw + // bytes straight from a file handle (kernel sendfile fast path). + path string + gi zran.Index gm sync.Mutex } +// zipFlagEncrypted is the general-purpose bit flag marking an encrypted entry, +// whose raw bytes are not the actual content. +const zipFlagEncrypted = 0x1 + +// hasRawFileAccess reports whether the entry's raw bytes can be served +// directly from the archive's file on disk. +func (e *gozipArchiveEntry) hasRawFileAccess() bool { + return e.path != "" && e.file.Flags&zipFlagEncrypted == 0 +} + +// streamRawFromFile copies length raw bytes of the entry, skipping the first +// start bytes, straight from a fresh handle on the archive file. The source +// reaching w is a bare *os.File (wrapped only by io.CopyN's *io.LimitedReader, +// which the runtime unwraps), so the kernel sendfile fast path is preserved +// when w is a network connection. handled is false when the fast path could +// not be attempted and the caller should fall back before writing anything. +func (e *gozipArchiveEntry) streamRawFromFile(w io.Writer, start int64, length int64) (n int64, err error, handled bool) { + offset, err := e.file.DataOffset() + if err != nil { + return 0, nil, false + } + f, err := os.Open(e.path) + if err != nil { + return 0, nil, false + } + defer f.Close() + if _, err := f.Seek(offset+start, io.SeekStart); err != nil { + return 0, nil, false + } + n, err = io.CopyN(w, f, length) + if err == io.EOF { + err = nil + } + return n, err, true +} + func (e *gozipArchiveEntry) Path() string { return path.Clean(e.file.Name) } @@ -44,10 +86,14 @@ func (e *gozipArchiveEntry) CRC32Checksum() *uint32 { } func (e *gozipArchiveEntry) CompressedAs(compressionMethod CompressionMethod) bool { - if compressionMethod != CompressionMethodDeflate { + switch compressionMethod { + case CompressionMethodDeflate: + return e.file.Method == zip.Deflate + case CompressionMethodStore: + return e.file.Method == zip.Store + default: return false } - return e.file.Method == zip.Deflate } // This is a special mode to minimize the number of reads from the underlying reader. @@ -62,6 +108,9 @@ func (e *gozipArchiveEntry) Read(start int64, end int64) ([]byte, error) { if end < start { return nil, errors.New("range not satisfiable") } + if start < 0 { + start = 0 + } minimizeReads := e.couldMinimizeReads() @@ -72,6 +121,15 @@ func (e *gozipArchiveEntry) Read(start int64, end int64) ([]byte, error) { if err != nil { return nil, err } + } else if e.CompressedLength() == 0 && e.file.Flags&zipFlagEncrypted == 0 { + // Raw bytes == content for stored entries; OpenRaw skips the CRC32 + // pass and returns a seekable reader, so a ranged read seeks to the + // range start instead of reading and discarding the prefix — which + // for a remote archive would download the whole prefix. + f, err = e.file.OpenRaw() + if err != nil { + return nil, err + } } else { rc, err := e.file.Open() if err != nil { @@ -169,6 +227,29 @@ func (e *gozipArchiveEntry) Stream(w io.Writer, start int64, end int64) (int64, if end < start { return -1, errors.New("range not satisfiable") } + if start < 0 { + start = 0 + } + + // For stored entries the raw bytes are the content, so they can be + // streamed directly from the archive file on disk, bypassing both the + // CRC32 pass of zip's checksum reader and userspace copy loops. + if e.CompressedLength() == 0 && e.hasRawFileAccess() { + size := int64(e.file.UncompressedSize64) + length := size + if !(start == 0 && end == 0) { + if start >= size { + return 0, nil + } + length = end - start + 1 + if length > size-start { + length = size - start + } + } + if n, err, handled := e.streamRawFromFile(w, start, length); handled { + return n, err + } + } minimizeReads := e.couldMinimizeReads() && start == 0 && end == 0 @@ -179,6 +260,13 @@ func (e *gozipArchiveEntry) Stream(w io.Writer, start int64, end int64) (int64, if err != nil { return -1, err } + } else if e.CompressedLength() == 0 && e.file.Flags&zipFlagEncrypted == 0 { + // Raw bytes == content for stored entries; OpenRaw skips the CRC32 + // pass and returns a seekable reader for cheap range starts. + f, err = e.file.OpenRaw() + if err != nil { + return -1, err + } } else { rc, err := e.file.Open() if err != nil { @@ -200,15 +288,22 @@ func (e *gozipArchiveEntry) Stream(w io.Writer, start int64, end int64) (int64, } if start == 0 && end == 0 { - return io.Copy(w, f) + n, err := copyRange(w, f, int64(e.file.UncompressedSize64)) + if err == io.EOF { + err = nil + } + return n, err } if start > 0 { - n, err := io.CopyN(io.Discard, f, start) - if err != nil { + if skr, ok := f.(io.Seeker); ok { + if _, err := skr.Seek(start, io.SeekStart); err != nil { + return -1, err + } + } else if n, err := io.CopyN(io.Discard, f, start); err != nil { return n, err } } - n, err := io.CopyN(w, f, end-start+1) + n, err := copyRange(w, f, end-start+1) if n > 0 && err == io.EOF { // Not EOF error if some data was read err = nil @@ -216,10 +311,78 @@ func (e *gozipArchiveEntry) Stream(w io.Writer, start int64, end int64) (int64, return n, err } +// streamCopyBufferSize is the buffer size used when copying entry bytes to a +// writer. The underlying archive is read at this granularity, and for a remote +// archive each read can become its own range request — io.Copy's default 32KB +// buffer would turn a large stream into one remote request per 32KB. It is +// deliberately larger than the default remote range-cache threshold (1 MiB) so +// that streamed media blocks are not needlessly copied into the range cache. +const streamCopyBufferSize = 2 << 20 // 2 MiB + +// copyBufferSize bounds the copy buffer to the number of bytes actually being +// copied, so streaming a small entry doesn't allocate the full buffer. +func copyBufferSize(length int64) int { + if length <= 0 { + return 1 + } + if length < streamCopyBufferSize { + return int(length) + } + return streamCopyBufferSize +} + +// copyRange copies exactly length bytes from r to w using a full-sized read +// per iteration, following io.CopyN semantics (io.EOF when r ends early). +// +// It deliberately avoids io.Copy/io.CopyBuffer: those delegate to the +// destination's ReaderFrom when available (an http.ResponseWriter does), which +// reads the source in its own small chunks — and every read of a remote +// archive can be its own range request. io.ReadFull guarantees each iteration +// asks the underlying reader for the whole buffer at once. +func copyRange(w io.Writer, r io.Reader, length int64) (int64, error) { + if length <= 0 { + return 0, nil + } + buf := make([]byte, copyBufferSize(length)) + var written int64 + for written < length { + n := int64(len(buf)) + if rem := length - written; rem < n { + n = rem + } + nr, rerr := io.ReadFull(r, buf[:n]) + if nr > 0 { + nw, werr := w.Write(buf[:nr]) + written += int64(nw) + if werr != nil { + return written, werr + } + if int64(nw) < int64(nr) { + return written, io.ErrShortWrite + } + } + if rerr == io.EOF || rerr == io.ErrUnexpectedEOF { + break // Source exhausted before the requested length + } + if rerr != nil { + return written, rerr + } + } + if written < length { + return written, io.EOF + } + return written, nil +} + func (e *gozipArchiveEntry) StreamCompressed(w io.Writer) (int64, error) { if e.file.Method != zip.Deflate { return -1, errors.New("not a compressed resource") } + if e.hasRawFileAccess() { + if n, err, handled := e.streamRawFromFile(w, 0, int64(e.file.CompressedSize64)); handled { + return n, err + } + } f, err := e.file.OpenRaw() if err != nil { return -1, err @@ -235,9 +398,13 @@ func (e *gozipArchiveEntry) StreamCompressedGzip(w io.Writer) (int64, error) { if e.file.UncompressedSize64 > math.MaxUint32 { return -1, errors.New("uncompressed size > 2^32 too large for GZIP") } - f, err := e.file.OpenRaw() - if err != nil { - return -1, err + var f io.Reader + if !e.hasRawFileAccess() { + var err error + f, err = e.file.OpenRaw() + if err != nil { + return -1, err + } } // Header @@ -249,7 +416,23 @@ func (e *gozipArchiveEntry) StreamCompressedGzip(w io.Writer) (int64, error) { return -1, errors.Wrap(err, "failed to write GZIP header") } - nn, err := io.Copy(w, f) + var nn int64 + if f == nil { + // The deflate body is copied straight from the archive file so the + // kernel sendfile fast path applies between header and trailer. + var handled bool + nn, err, handled = e.streamRawFromFile(w, 0, int64(e.file.CompressedSize64)) + if !handled { + var oerr error + f, oerr = e.file.OpenRaw() + if oerr != nil { + return int64(n), oerr + } + nn, err = io.Copy(w, f) + } + } else { + nn, err = io.Copy(w, f) + } if err != nil { return int64(n), errors.Wrap(err, "failed copying deflated bytes") } @@ -322,6 +505,10 @@ type gozipArchive struct { closer func() error minimizeReads bool + // path is the filesystem path of the archive when it was opened from a + // local file, or empty otherwise. See gozipArchiveEntry.path. + path string + // nameIndex maps each entry's cleaned path to its *zip.File, giving O(1) // lookups instead of a linear scan. It's built once, lazily, on the first // Entry call that misses the wrapper cache. @@ -356,6 +543,7 @@ func (a *gozipArchive) wrap(cleanPath string, f *zip.File) *gozipArchiveEntry { entry := &gozipArchiveEntry{ file: f, minimizeReads: a.minimizeReads, + path: a.path, } actual, _ := a.cachedEntries.LoadOrStore(cleanPath, entry) return actual.(*gozipArchiveEntry) @@ -419,7 +607,11 @@ func (e gozipArchiveFactory) Open(filepath string, password string) (Archive, er if err != nil { return nil, err } - return NewGoZIPArchive(&rc.Reader, rc.Close, false), nil + return &gozipArchive{ + zip: &rc.Reader, + closer: rc.Close, + path: filepath, + }, nil } // OpenBytes implements ArchiveFactory diff --git a/pkg/archive/archive_zip_stream_test.go b/pkg/archive/archive_zip_stream_test.go new file mode 100644 index 00000000..cf90f863 --- /dev/null +++ b/pkg/archive/archive_zip_stream_test.go @@ -0,0 +1,190 @@ +package archive + +import ( + "archive/zip" + "bytes" + "compress/flate" + "compress/gzip" + "io" + "os" + "path/filepath" + "testing" + + "github.com/readium/go-toolkit/pkg/util/url" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Deterministic, poorly-compressible payload so stored/deflated sizes differ +// and range checks catch off-by-one errors. +func streamTestPayload(size int) []byte { + data := make([]byte, size) + for i := range data { + data[i] = byte(i*7 + i>>8 + i>>13) + } + return data +} + +const ( + storedName = "media/stored.bin" + deflateName = "text/deflated.txt" +) + +var ( + storedPayload = streamTestPayload(100_000) + deflatePayload = bytes.Repeat([]byte("readium go-toolkit stream test payload. "), 5000) +) + +func makeStreamTestZip(t *testing.T) []byte { + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + w, err := zw.CreateHeader(&zip.FileHeader{Name: storedName, Method: zip.Store}) + require.NoError(t, err) + _, err = w.Write(storedPayload) + require.NoError(t, err) + + w, err = zw.CreateHeader(&zip.FileHeader{Name: deflateName, Method: zip.Deflate}) + require.NoError(t, err) + _, err = w.Write(deflatePayload) + require.NoError(t, err) + + require.NoError(t, zw.Close()) + return buf.Bytes() +} + +// withStreamTestArchives runs the callback against the same zip opened from a +// local file (raw-file fast paths) and from memory (fallback paths). +func withStreamTestArchives(t *testing.T, callback func(t *testing.T, a Archive)) { + data := makeStreamTestZip(t) + + fpath := filepath.Join(t.TempDir(), "stream.zip") + require.NoError(t, os.WriteFile(fpath, data, 0o600)) + u, err := url.FromFilepath(fpath) + require.NoError(t, err) + fileArchive, err := DefaultArchiveFactory{}.Open(t.Context(), u, "") + require.NoError(t, err) + defer fileArchive.Close() + + bytesArchive, err := DefaultArchiveFactory{}.OpenBytes(t.Context(), data, "") + require.NoError(t, err) + defer bytesArchive.Close() + + t.Run("file", func(t *testing.T) { callback(t, fileArchive) }) + t.Run("bytes", func(t *testing.T) { callback(t, bytesArchive) }) +} + +func TestZipStreamMatchesContent(t *testing.T) { + entries := map[string][]byte{ + storedName: storedPayload, + deflateName: deflatePayload, + } + ranges := []struct { + name string + start, end int64 + }{ + {"full", 0, 0}, + {"prefix", 0, 9}, + {"middle", 1000, 4999}, + {"suffixExact", int64(len(storedPayload)) - 100, int64(len(storedPayload)) - 1}, + {"clampedEnd", 5, int64(len(storedPayload)) + 500}, + {"negativeStart", -5, 9}, + } + + withStreamTestArchives(t, func(t *testing.T, a Archive) { + for name, content := range entries { + entry, err := a.Entry(name) + require.NoError(t, err) + + for _, r := range ranges { + var b bytes.Buffer + n, err := entry.Stream(&b, r.start, r.end) + require.NoError(t, err, "%s %s", name, r.name) + + start, end := r.start, r.end + if start < 0 { + start = 0 + } + expected := content + if !(r.start == 0 && r.end == 0) { + if end > int64(len(content))-1 { + end = int64(len(content)) - 1 + } + expected = content[start : end+1] + } + assert.EqualValues(t, len(expected), n, "%s %s", name, r.name) + assert.True(t, bytes.Equal(expected, b.Bytes()), "%s %s content mismatch", name, r.name) + + // Stream and Read must agree + rb, err := entry.Read(r.start, r.end) + require.NoError(t, err, "%s %s", name, r.name) + assert.True(t, bytes.Equal(rb, b.Bytes()), "%s %s Stream/Read mismatch", name, r.name) + } + } + }) +} + +func TestZipStreamStartPastEnd(t *testing.T) { + withStreamTestArchives(t, func(t *testing.T, a Archive) { + entry, err := a.Entry(storedName) + require.NoError(t, err) + + var b bytes.Buffer + n, err := entry.Stream(&b, int64(len(storedPayload))+10, int64(len(storedPayload))+20) + if err != nil { + // Fallback paths surface EOF here; either way nothing is written. + assert.ErrorIs(t, err, io.EOF) + } + assert.LessOrEqual(t, n, int64(0)) + assert.Empty(t, b.Bytes()) + }) +} + +func TestZipStreamCompressed(t *testing.T) { + withStreamTestArchives(t, func(t *testing.T, a Archive) { + entry, err := a.Entry(deflateName) + require.NoError(t, err) + + var b bytes.Buffer + n, err := entry.Stream(&b, 0, 0) + require.NoError(t, err) + assert.EqualValues(t, len(deflatePayload), n) + + var cb bytes.Buffer + cn, err := entry.StreamCompressed(&cb) + require.NoError(t, err) + assert.EqualValues(t, entry.CompressedLength(), cn) + assert.EqualValues(t, entry.CompressedLength(), cb.Len()) + + // The streamed bytes must be the actual deflate stream of the content + fr := flate.NewReader(bytes.NewReader(cb.Bytes())) + inflated, err := io.ReadAll(fr) + require.NoError(t, err) + assert.True(t, bytes.Equal(deflatePayload, inflated)) + + // Stored entries have no compressed representation + stored, err := a.Entry(storedName) + require.NoError(t, err) + _, err = stored.StreamCompressed(&bytes.Buffer{}) + assert.Error(t, err) + }) +} + +func TestZipStreamCompressedGzip(t *testing.T) { + withStreamTestArchives(t, func(t *testing.T, a Archive) { + entry, err := a.Entry(deflateName) + require.NoError(t, err) + + var b bytes.Buffer + n, err := entry.StreamCompressedGzip(&b) + require.NoError(t, err) + assert.EqualValues(t, b.Len(), n) + + gr, err := gzip.NewReader(bytes.NewReader(b.Bytes())) + require.NoError(t, err) + gunzipped, err := io.ReadAll(gr) + require.NoError(t, err) + require.NoError(t, gr.Close()) + assert.True(t, bytes.Equal(deflatePayload, gunzipped)) + }) +} diff --git a/pkg/archive/remote.go b/pkg/archive/remote.go index 06478361..d2e4e843 100644 --- a/pkg/archive/remote.go +++ b/pkg/archive/remote.go @@ -182,19 +182,10 @@ func (r *remoteZIPAdapter) ReadAt(p []byte, off int64) (int, error) { // e.g. with Go, where if you write a streaming ZIP, the size is not known in advance. // We can still at least cache the file header - r.cacheMutex.Lock() - if len(r.cachedRanges) >= int(r.cacheCountThreshold) { - // Remove the oldest range - r.cachedRanges = r.cachedRanges[1:] - } - - r.cachedRanges = append(r.cachedRanges, readRange{ - HeaderOffset: off, - Header: fileHeaderBuf, - }) - r.cacheMutex.Unlock() + r.cacheHeader(off, fileHeaderBuf) } else if compressedSize == 0xFFFFFFFF && uncompressedSize == 0xFFFFFFFF { - // ZIP64 is not supported by this routine + // ZIP64 is not supported by this routine, but the header can still be cached + r.cacheHeader(off, fileHeaderBuf) } else { if compressionMethod == zip.Store { // File is uncompressed @@ -207,7 +198,13 @@ func (r *remoteZIPAdapter) ReadAt(p []byte, off int64) (int, error) { // Now the important part - we precache the actual file! // ...but only if it's not too big - if int64(bodySize) <= r.cacheSizeThreshold { + if int64(bodySize) > r.cacheSizeThreshold { + // Too big to precache. Still remember the header, so that + // subsequent opens of this entry (every ranged read of a + // large media file opens it again) are served from memory + // instead of a new remote request. + r.cacheHeader(off, fileHeaderBuf) + } else { // Remaining local file headers are needed to get the total size of useless stuff filenameLength := binary.LittleEndian.Uint16(b[8:]) extraFieldLength := binary.LittleEndian.Uint16(b[10:]) @@ -252,8 +249,13 @@ func (r *remoteZIPAdapter) ReadAt(p []byte, off int64) (int, error) { } } copy(p, fileHeaderBuf[:]) // Copy the 30 read bytes - io.Copy(io.Discard, rdr) // Discard the rest of the read - rdr.Close() // Then close it + // The read was open-ended (the body size wasn't known upfront), so it + // must not be drained to EOF: everything left in it is the rest of + // the archive, which a remote reader would download in full. Drain a + // small amount so a keep-alive connection stays reusable when the + // remainder is tiny, then close, aborting the rest of the transfer. + io.CopyN(io.Discard, rdr, maxDrainBytes) + rdr.Close() } else { // Check all the cache ranges to see if what we're looking for is somewhere inside a cached range // This is especially useful when doing a range read / stream of e.g. 4096-byte chunks @@ -313,6 +315,25 @@ func (r *remoteZIPAdapter) ReadAt(p []byte, off int64) (int, error) { return n, nil } +// maxDrainBytes bounds how much of an open-ended remote read is drained before +// closing it, to keep the connection reusable when the remainder is small. +const maxDrainBytes = 4 << 10 // 4 KiB + +// cacheHeader remembers a local file header so subsequent opens of the same +// entry serve it from memory instead of performing a new remote read. +func (r *remoteZIPAdapter) cacheHeader(off int64, header [30]byte) { + r.cacheMutex.Lock() + if len(r.cachedRanges) >= int(r.cacheCountThreshold) { + // Remove the oldest range + r.cachedRanges = r.cachedRanges[1:] + } + r.cachedRanges = append(r.cachedRanges, readRange{ + HeaderOffset: off, + Header: header, + }) + r.cacheMutex.Unlock() +} + func (r *remoteZIPAdapter) makeReady() { r.zipReady = true r.zipTail = nil diff --git a/pkg/archive/remote_test.go b/pkg/archive/remote_test.go new file mode 100644 index 00000000..3368bc84 --- /dev/null +++ b/pkg/archive/remote_test.go @@ -0,0 +1,241 @@ +package archive + +import ( + "archive/zip" + "bytes" + "context" + "io" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// recordedRead captures one ReadRange call and how many bytes were actually +// consumed from the reader it returned. +type recordedRead struct { + offset int64 + length int64 + consumed int64 +} + +// countingRemoteReader is an in-memory RemoteArchiveReader that records every +// range request and the number of bytes read from it, so tests can assert how +// much a remote archive would really have transferred. +type countingRemoteReader struct { + data []byte + reads []*recordedRead +} + +func (r *countingRemoteReader) Size() int64 { + return int64(len(r.data)) +} + +func (r *countingRemoteReader) ReadRange(ctx context.Context, offset, length int64) (io.ReadCloser, error) { + rec := &recordedRead{offset: offset, length: length} + r.reads = append(r.reads, rec) + end := int64(len(r.data)) + if length >= 0 && offset+length < end { + end = offset + length + } + if offset > end { + offset = end + } + return &countingReadCloser{r: bytes.NewReader(r.data[offset:end]), rec: rec}, nil +} + +func (r *countingRemoteReader) reset() { + r.reads = nil +} + +func (r *countingRemoteReader) totalConsumed() int64 { + var total int64 + for _, rec := range r.reads { + total += rec.consumed + } + return total +} + +type countingReadCloser struct { + r *bytes.Reader + rec *recordedRead +} + +func (c *countingReadCloser) Read(p []byte) (int, error) { + n, err := c.r.Read(p) + c.rec.consumed += int64(n) + return n, err +} + +func (c *countingReadCloser) Close() error { + return nil +} + +// buildRemoteTestZIP creates an in-memory ZIP with a small stored entry, a +// large stored entry (media-like), and a small deflated entry, returning the +// archive bytes and the content of the large entry. +func buildRemoteTestZIP(t *testing.T, bigSize int) (zipBytes []byte, big []byte) { + t.Helper() + big = make([]byte, bigSize) + for i := range big { + big[i] = byte((i * 7) % 251) + } + + var buf bytes.Buffer + zw := zip.NewWriter(&buf) + + small, err := zw.CreateHeader(&zip.FileHeader{Name: "small.bin", Method: zip.Store}) + require.NoError(t, err) + smallData := bytes.Repeat([]byte("readium!"), 1280) // 10 KiB + _, err = small.Write(smallData) + require.NoError(t, err) + + bigW, err := zw.CreateHeader(&zip.FileHeader{Name: "big.bin", Method: zip.Store}) + require.NoError(t, err) + _, err = bigW.Write(big) + require.NoError(t, err) + + defl, err := zw.CreateHeader(&zip.FileHeader{Name: "small.txt", Method: zip.Deflate}) + require.NoError(t, err) + _, err = defl.Write([]byte("hello deflated world")) + require.NoError(t, err) + + require.NoError(t, zw.Close()) + return buf.Bytes(), big +} + +// openCountingArchive opens the ZIP the same way the remote (HTTP/S3/GCS) +// archive factories do: through a remoteZIPAdapter with minimizeReads set. +func openCountingArchive(t *testing.T, zipBytes []byte) (*countingRemoteReader, Archive) { + t.Helper() + reader := &countingRemoteReader{data: zipBytes} + rdr := newRemoteZIPAdapter(reader, RemoteArchiveConfig{ + Timeout: time.Minute, + CacheSizeThreshold: 1024 * 1024, + CacheCountThreshold: 32, + CacheAllThreshold: 1024, // Below the archive size, so nothing is fully cached + }) + zr, err := zip.NewReader(rdr, int64(len(zipBytes))) + require.NoError(t, err) + rdr.makeReady() + return reader, &gozipArchive{zip: zr, minimizeReads: true, closer: rdr.Close} +} + +// A ranged Read of a stored entry must transfer roughly the requested range: +// no reading-and-discarding of the range's prefix, and no draining of the rest +// of the archive after the header probe. +func TestRemoteStoredEntryRangedReadBounded(t *testing.T) { + zipBytes, big := buildRemoteTestZIP(t, 5<<20) + reader, a := openCountingArchive(t, zipBytes) + + entry, err := a.Entry("big.bin") + require.NoError(t, err) + reader.reset() + + // 100 KiB read starting 4 MiB into the entry + start := int64(4 << 20) + length := int64(100 << 10) + data, err := entry.Read(start, start+length-1) + require.NoError(t, err) + assert.Equal(t, big[start:start+length], data) + + // Header probe (30 bytes + bounded drain) plus the requested range; + // before the fixes this consumed the 4 MiB prefix and the archive tail. + consumed := reader.totalConsumed() + assert.LessOrEqual(t, consumed, length+maxDrainBytes+1024, + "ranged read should not transfer (much) more than the range itself, got %d bytes", consumed) +} + +// plainWriter is a writer that deliberately does NOT implement io.ReaderFrom, +// mirroring how an http.ResponseWriter behind middleware may behave, so the +// copy loop's own read granularity is what is measured. +type plainWriter struct { + buf bytes.Buffer +} + +func (w *plainWriter) Write(p []byte) (int, error) { + return w.buf.Write(p) +} + +// Streaming a range of a stored entry must fetch it in large chunks (one +// remote request per copy buffer) rather than one request per 32 KiB, and a +// second stream of the same entry must not re-fetch the entry header. +func TestRemoteStoredEntryStreamChunked(t *testing.T) { + zipBytes, big := buildRemoteTestZIP(t, 5<<20) + reader, a := openCountingArchive(t, zipBytes) + + entry, err := a.Entry("big.bin") + require.NoError(t, err) + reader.reset() + + // 3 MiB range: expect one 2 MiB chunk plus one 1 MiB chunk + start := int64(1 << 20) + length := int64(3 << 20) + var buf plainWriter + n, err := entry.Stream(&buf, start, start+length-1) + require.NoError(t, err) + assert.Equal(t, length, n) + assert.Equal(t, big[start:start+length], buf.buf.Bytes()) + + var bodyReads int + for _, rec := range reader.reads { + if rec.length >= 1<<20 { + bodyReads++ + } + } + assert.Equal(t, 2, bodyReads, "3 MiB should stream as two large chunks, got reads: %+v", reader.reads) + // Header probe + two chunks; a flood of small reads (e.g. one per 32 KiB) + // would mean the copy buffer is being bypassed. + assert.LessOrEqual(t, len(reader.reads), 3, "unexpected extra reads: %+v", reader.reads) + assert.LessOrEqual(t, reader.totalConsumed(), length+maxDrainBytes+1024) + + // Second stream: the entry header is now cached (and chunks small enough + // for the range cache may be too), so at most the body chunks are fetched + // again — crucially, no extra header request. + reader.reset() + buf.buf.Reset() + _, err = entry.Stream(&buf, start, start+length-1) + require.NoError(t, err) + assert.Equal(t, big[start:start+length], buf.buf.Bytes()) + assert.LessOrEqual(t, len(reader.reads), 2, "header should be cached after the first stream, got reads: %+v", reader.reads) +} + +// Fully reading a small stored entry precaches it via a single open-ended +// request, which must not drain the remainder of the archive. +func TestRemoteSmallEntryReadDoesNotDrainArchive(t *testing.T) { + zipBytes, _ := buildRemoteTestZIP(t, 5<<20) + reader, a := openCountingArchive(t, zipBytes) + + entry, err := a.Entry("small.bin") + require.NoError(t, err) + reader.reset() + + data, err := entry.Read(0, 0) + require.NoError(t, err) + assert.Equal(t, bytes.Repeat([]byte("readium!"), 1280), data) + + // Entry body is 10 KiB; the big entry follows it in the archive. Before + // the drain fix this transferred the entire archive tail (> 5 MiB). + consumed := reader.totalConsumed() + assert.LessOrEqual(t, consumed, int64(10<<10)+maxDrainBytes+1024, + "reading a small entry should not drain the archive tail, got %d bytes", consumed) +} + +// Deflated entries are still read correctly through the adapter after the +// stored-entry changes. +func TestRemoteDeflatedEntryRead(t *testing.T) { + zipBytes, _ := buildRemoteTestZIP(t, 2<<20) + _, a := openCountingArchive(t, zipBytes) + + entry, err := a.Entry("small.txt") + require.NoError(t, err) + data, err := entry.Read(0, 0) + require.NoError(t, err) + assert.Equal(t, []byte("hello deflated world"), data) + + // Ranged read of a deflated entry + data, err = entry.Read(6, 13) + require.NoError(t, err) + assert.Equal(t, []byte("deflated"), data) +} diff --git a/pkg/fetcher/fetcher_archive.go b/pkg/fetcher/fetcher_archive.go index 7afd95e3..169a6a35 100644 --- a/pkg/fetcher/fetcher_archive.go +++ b/pkg/fetcher/fetcher_archive.go @@ -191,6 +191,16 @@ func (r *entryResource) Stream(ctx context.Context, w io.Writer, start int64, en return -1, Other(err) } +// HasEfficientStream implements EfficientStreamer. Stored (uncompressed) +// entries are streamed by seeking straight to the requested range of the +// archive, so Stream reads only what is requested even from a remote archive. +// Deflate-compressed entries are excluded: a ranged Stream has to decompress +// from the start of the entry on every call, whereas Read uses a random-access +// index that persists across calls. +func (r *entryResource) HasEfficientStream() bool { + return r.entry.CompressedAs(archive.CompressionMethodStore) +} + // CompressedAs implements CompressedResource func (r *entryResource) CompressedAs(compressionMethod archive.CompressionMethod) bool { return r.entry.CompressedAs(compressionMethod) diff --git a/pkg/fetcher/fetcher_archive_test.go b/pkg/fetcher/fetcher_archive_test.go index e1d357b2..e7dac3c8 100644 --- a/pkg/fetcher/fetcher_archive_test.go +++ b/pkg/fetcher/fetcher_archive_test.go @@ -149,3 +149,21 @@ func TestArchiveFetcherAddsProperties(t *testing.T) { }, resource.Properties()) }) } + +// entryResource advertises efficient streaming for stored entries only: +// deflated entries must keep using Read for ranged access from remote +// sources, since their ranged Stream decompresses from the entry start. +// This also pins CompressedAs(CompressionMethodStore) answering correctly. +func TestArchiveFetcherEfficientStream(t *testing.T) { + withArchiveFetcher(t, func(a *ArchiveFetcher) { + stored := a.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString("mimetype", false)}) + es, ok := stored.(EfficientStreamer) + require.True(t, ok) + assert.True(t, es.HasEfficientStream(), "stored entry should advertise efficient streaming") + + deflated := a.Get(t.Context(), manifest.Link{Href: manifest.MustNewHREFFromString("EPUB/cover.xhtml", false)}) + es, ok = deflated.(EfficientStreamer) + require.True(t, ok) + assert.False(t, es.HasEfficientStream(), "deflated entry should not advertise efficient streaming") + }) +} diff --git a/pkg/fetcher/fetcher_file.go b/pkg/fetcher/fetcher_file.go index 8db1ddc9..72c8f90f 100644 --- a/pkg/fetcher/fetcher_file.go +++ b/pkg/fetcher/fetcher_file.go @@ -134,7 +134,7 @@ type FileResource struct { link manifest.Link path string - mu sync.Mutex // guards file and sequential (offset-based) access to it + mu sync.Mutex // guards lazily opening and closing file file *os.File } @@ -203,16 +203,16 @@ func (r *FileResource) Read(ctx context.Context, start int64, end int64) ([]byte return nil, ex } if start == 0 && end == 0 { - r.mu.Lock() - defer r.mu.Unlock() - if _, err := f.Seek(0, io.SeekStart); err != nil { + fi, err := f.Stat() + if err != nil { return nil, Other(err) } - data, err := io.ReadAll(f) - if err != nil { + data := make([]byte, fi.Size()) + n, err := f.ReadAt(data, 0) + if err != nil && err != io.EOF { return nil, Other(err) } - return data, nil + return data[:n], nil } data := make([]byte, end-start+1) n, err := f.ReadAt(data, start) @@ -224,7 +224,6 @@ func (r *FileResource) Read(ctx context.Context, start int64, end int64) ([]byte // Stream implements Resource func (r *FileResource) Stream(ctx context.Context, w io.Writer, start int64, end int64) (int64, *ResourceError) { - defer runtime.KeepAlive(r) if end < start { err := RangeNotSatisfiable(errors.New("end of range smaller than start")) return -1, err @@ -232,24 +231,36 @@ func (r *FileResource) Stream(ctx context.Context, w io.Writer, start int64, end if start < 0 { start = 0 } - f, ex := r.open() - if ex != nil { - return -1, ex + // Streaming uses a private handle per call: the offset-based access below + // never contends with concurrent streams of the same resource, and the + // source stays a bare *os.File (wrapped only by io.CopyN's + // *io.LimitedReader, which the runtime unwraps), preserving the kernel + // sendfile fast path when w is a network connection. An io.SectionReader + // here would force a userspace copy loop. + f, err := os.Open(r.path) + if err != nil { + return -1, OsErrorToException(err) + } + defer f.Close() + fi, err := f.Stat() + if err != nil { + return -1, Other(err) + } + if fi.IsDir() { + return -1, NotFound(errors.New("is a directory")) } if start == 0 && end == 0 { - r.mu.Lock() - defer r.mu.Unlock() - if _, err := f.Seek(0, io.SeekStart); err != nil { - return -1, Other(err) - } n, err := io.Copy(w, f) if err != nil { return -1, Other(err) } return n, nil } - n, err := io.Copy(w, io.NewSectionReader(f, start, end-start+1)) - if err != nil { + if _, err := f.Seek(start, io.SeekStart); err != nil { + return -1, Other(err) + } + n, err := io.CopyN(w, f, end-start+1) + if err != nil && err != io.EOF { return n, Other(err) } return n, nil diff --git a/pkg/fetcher/fetcher_gcs.go b/pkg/fetcher/fetcher_gcs.go index 5e8cd6e3..97dade52 100644 --- a/pkg/fetcher/fetcher_gcs.go +++ b/pkg/fetcher/fetcher_gcs.go @@ -243,6 +243,12 @@ func (r *gcsResource) Stream(ctx context.Context, w io.Writer, start int64, end return n, nil } +// HasEfficientStream implements EfficientStreamer. Stream performs a single +// ranged object read and pipes the data through as it arrives. +func (r *gcsResource) HasEfficientStream() bool { + return true +} + // Length implements Resource func (r *gcsResource) Length(ctx context.Context) (int64, *ResourceError) { attrs, rerr := r.attrs(ctx) diff --git a/pkg/fetcher/fetcher_http.go b/pkg/fetcher/fetcher_http.go index a097a44b..0a3a5f88 100644 --- a/pkg/fetcher/fetcher_http.go +++ b/pkg/fetcher/fetcher_http.go @@ -189,6 +189,7 @@ func (r *httpResource) Read(ctx context.Context, start int64, end int64) ([]byte if err != nil { return nil, Other(err) } + defer resp.Body.Close() if (start == 0 && end == 0 && resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusPartialContent) || ((start != 0 || end != 0) && resp.StatusCode != http.StatusPartialContent) { ex := httpStatusToException(resp.StatusCode) @@ -197,7 +198,6 @@ func (r *httpResource) Read(ctx context.Context, start int64, end int64) ([]byte } return nil, ex } - defer resp.Body.Close() var data []byte if resp.ContentLength >= 0 { @@ -237,14 +237,17 @@ func (r *httpResource) Stream(ctx context.Context, w io.Writer, start int64, end if err != nil { return -1, Other(err) } - if resp.StatusCode != http.StatusPartialContent { + defer resp.Body.Close() + + // A request without a Range header (whole resource) is answered with 200, + // a ranged request must be answered with 206. + if (start == 0 && end == 0 && resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusPartialContent) || ((start != 0 || end != 0) && resp.StatusCode != http.StatusPartialContent) { ex := httpStatusToException(resp.StatusCode) if ex == nil { return -1, Other(errors.New("unexpected HTTP status code: " + strconv.Itoa(resp.StatusCode))) } return -1, ex } - defer resp.Body.Close() n, err := io.Copy(w, resp.Body) if err != nil { @@ -253,6 +256,12 @@ func (r *httpResource) Stream(ctx context.Context, w io.Writer, start int64, end return n, nil } +// HasEfficientStream implements EfficientStreamer. Stream performs a single +// ranged HTTP request and pipes the response body through as it arrives. +func (r *httpResource) HasEfficientStream() bool { + return true +} + // Length implements Resource func (r *httpResource) Length(ctx context.Context) (int64, *ResourceError) { size, rerr := r.size(ctx) diff --git a/pkg/fetcher/fetcher_http_test.go b/pkg/fetcher/fetcher_http_test.go index 6f762e29..dca17804 100644 --- a/pkg/fetcher/fetcher_http_test.go +++ b/pkg/fetcher/fetcher_http_test.go @@ -1,8 +1,12 @@ package fetcher import ( + "bytes" "net/http" + "net/http/httptest" + "sync" "testing" + "time" "github.com/readium/go-toolkit/pkg/manifest" "github.com/readium/go-toolkit/pkg/util/url" @@ -36,3 +40,58 @@ func TestHTTPFetcherContainsPath(t *testing.T) { assert.Equal(t, tt.url, hres.url.String()) } } + +// The bare-file remote resources advertise efficient streaming; archive +// entries advertise it based on their compression method. +var ( + _ EfficientStreamer = (*httpResource)(nil) + _ EfficientStreamer = (*s3Resource)(nil) + _ EfficientStreamer = (*gcsResource)(nil) + _ EfficientStreamer = (*entryResource)(nil) +) + +// Stream must perform exactly one upstream request per call: an un-ranged GET +// (answered with 200) when streaming the whole resource, and a single ranged +// GET (answered with 206) when streaming a range. +func TestHTTPResourceStream(t *testing.T) { + payload := make([]byte, 100_000) + for i := range payload { + payload[i] = byte(i % 251) + } + + var mu sync.Mutex + var ranges []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + ranges = append(ranges, r.Header.Get("Range")) + mu.Unlock() + http.ServeContent(w, r, "file.bin", time.Time{}, bytes.NewReader(payload)) + })) + defer srv.Close() + + u, err := url.AbsoluteURLFromString(srv.URL + "/file.bin") + require.NoError(t, err) + res := NewHTTPResource(manifest.Link{}, srv.Client(), u) + + // Whole resource: no Range header is sent, and the origin's 200 response + // must be accepted. + var buf bytes.Buffer + n, rerr := res.Stream(t.Context(), &buf, 0, 0) + require.Nil(t, rerr) + assert.Equal(t, int64(len(payload)), n) + assert.Equal(t, payload, buf.Bytes()) + mu.Lock() + assert.Equal(t, []string{""}, ranges) + ranges = nil + mu.Unlock() + + // Ranged: a single ranged request, answered with 206. + buf.Reset() + n, rerr = res.Stream(t.Context(), &buf, 100, 199) + require.Nil(t, rerr) + assert.Equal(t, int64(100), n) + assert.Equal(t, payload[100:200], buf.Bytes()) + mu.Lock() + assert.Equal(t, []string{"bytes=100-199"}, ranges) + mu.Unlock() +} diff --git a/pkg/fetcher/fetcher_s3.go b/pkg/fetcher/fetcher_s3.go index 45984286..5fec1430 100644 --- a/pkg/fetcher/fetcher_s3.go +++ b/pkg/fetcher/fetcher_s3.go @@ -256,6 +256,12 @@ func (r *s3Resource) Stream(ctx context.Context, w io.Writer, start int64, end i return n, nil } +// HasEfficientStream implements EfficientStreamer. Stream performs a single +// ranged GetObject request and pipes the response body through as it arrives. +func (r *s3Resource) HasEfficientStream() bool { + return true +} + // Length implements Resource func (r *s3Resource) Length(ctx context.Context) (int64, *ResourceError) { head, rerr := r.head(ctx) diff --git a/pkg/fetcher/traits.go b/pkg/fetcher/traits.go index 8817eff3..84d229c1 100644 --- a/pkg/fetcher/traits.go +++ b/pkg/fetcher/traits.go @@ -16,3 +16,20 @@ type CompressedResource interface { ReadCompressedGzip(ctx context.Context) ([]byte, *ResourceError) CRC32Checksum(ctx context.Context) *uint32 } + +// EfficientStreamer is implemented by resources that can report whether their +// Stream method retrieves just the requested byte range from the underlying +// source with bounded per-request overhead — no reading and discarding of the +// range's prefix, and no degradation into one remote request per tiny chunk. +// +// When HasEfficientStream reports true, Stream is at least as efficient as +// Read even when the resource is backed by a remote source (HTTP, S3, GCS), +// while using bounded memory and delivering the first byte as soon as it is +// available. When it reports false — e.g. for a deflate-compressed entry in an +// archive, where a ranged Stream must decompress from the start of the entry — +// callers serving remote content should prefer Read for ranged access. +type EfficientStreamer interface { + // HasEfficientStream reports whether Stream retrieves only the requested + // range from the underlying source. See [EfficientStreamer]. + HasEfficientStream() bool +}