Skip to content

Report io.EOF from UnionFile.ReadAt - #653

Open
youdie006 wants to merge 1 commit into
spf13:masterfrom
youdie006:unionfile-readat-eof
Open

Report io.EOF from UnionFile.ReadAt#653
youdie006 wants to merge 1 commit into
spf13:masterfrom
youdie006:unionfile-readat-eof

Conversation

@youdie006

Copy link
Copy Markdown

UnionFile.ReadAt returns a nil error on a short read, so callers that rely on io.EOF to stop
never do.

io.ReaderAt, verbatim from $GOROOT/src/io/io.go:210:

When ReadAt returns n < len(p), it returns a non-nil error explaining why more bytes were not
returned. In this respect, ReadAt is stricter than Read.

Same calls through three implementations, 10-byte file:

                  ReadAt(buf[8], 6)      ReadAt(buf[8], 10)
os.File           n=4 err=EOF            n=0 err=EOF
afero mem.File    n=4 err=EOF            n=0 err=EOF
afero UnionFile   n=4 err=<nil>          n=0 err=<nil>

The (0, nil) case is what bites in practice. io.NewSectionReader — the natural thing to write
when the exact size is not known — cannot terminate:

os.File          terminated after 3 calls
afero mem.File   terminated after 3 calls
afero UnionFile  still going at 200 calls

Both of the other columns are afero's own code, so this is not an os-vs-mem difference.

Cause

unionFile.go:70:

n, err := f.Layer.ReadAt(s, o)
if (err == nil || err == io.EOF) && f.Base != nil {
    _, err = f.Base.Seek(o+int64(n), io.SeekStart)
}
return n, err

The assignment replaces the layer's io.EOF with the seek's nil. Read, sixteen lines above, gets
this right and carries the comment saying why:

if _, seekErr := f.Base.Seek(int64(n), io.SeekCurrent); seekErr != nil {
    // only overwrite err in case the seek fails: we need to
    // report an eventual io.EOF to the caller
    err = seekErr
}

Change

Five lines, mirroring Read. A test is added to composite_test.go covering the short read and the
read at EOF. Red with only unionFile.go reverted:

composite_test.go:537: ReadAt short read = (4, <nil>), want (4, EOF)
composite_test.go:540: ReadAt at EOF = (0, <nil>), want (0, EOF)

Green with the change; go test ./... passes across all packages and gofmt -l lists neither file.

Reachable through the public API via NewCacheOnReadFs(base, layer, 0).OpenFile(name, os.O_RDWR, perm), which returns a *UnionFile (cacheOnReadFs.go:233).


Disclosure: prepared with AI assistance; I verified the three-way comparison, the section-reader
behaviour and the red/green runs myself.

ReadAt overwrote the layer's error with the result of the base seek, so a
short read came back with a nil error. Read already guards the seek for
exactly this reason; ReadAt now does the same.
@CLAassistant

CLAassistant commented Sep 3, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants