See race condition issues too: https://go-review.googlesource.com/c/go/+/500476 ✨

I think the simplest way to do that is to always return a new section reader and try the unwrapping in .Read, however this might be expensive or require extra new synchronisation, making the sectionreader object bigger.

Can you explain where the race is in your change? I can't see the semantic subtlety, it seems to me that if the original nested SectionReader logic is race-free, then the change should be as well? It only touches r, base and limit from the original SectionReader, which all seem to be constant after construction. What am I missing?

https://go-review.googlesource.com/c/go/+/500476/comments/aef8d7b0_b285c620

However now that I checked, .Read and .Seek writes to .off, all the methods read from .base and .limit, the unwrapping also read .base and .limit so actually it looks fine.

I don't think this is possible due to this:

const s = "hello world"
b := strings.NewReader(s)
s1 := io.NewSectionReader(b, 3, len(s)-3)
s2 := io.NewSectionReader(b, 1, len(s)-4)

s1.Seek(2, io.SeekStart)
s2.Read(buf) // reads "world" because s1.base (3) + s1.off (2) + s2.base (1) = 6, so "hello " is chopped off.

I don't think that's right. SectionReader only ever calls ReadAt on the underlying r, and Read and Seek are implemented in terms of ReadAt + locally tracking s.off. If you construct two nested SectionReaders today, their Seek/Read cursors are unrelated and are free to diverge. Decoupling the two SectionReaders from each other in NewSectionReader doesn't seem like it changes those semantics at all, from the user's POV both SectionReaders still behave identically.

In particular, in the example you gave, s2's read is not influenced by s1.off, because s2.Read invokes s1.ReadAt with an absolute position, and s1.ReadAt doesn't incorporate s1.off into its calculations.

However, it does interact with #61870 : once SectionReader.Outer is released to stable, flattening out intermediate SectionReaders is a semantic change:

sr := io.NewSectionReader(r, 42, 100)
sr2 := io.NewSectionReader(sr, 50, 10)

Without this proposal, sr.Outer() returns r, whereas sr2.Outer() returns sr. With this proposal, both return r (with appropriately adjusted offset/limit, of course). Further, in 1ce8a3f, Outer explicitly documents that it always returns the ReaderAt that was passed to NewSectionReader.

I don't think the two necessarily interact. Isn't it enough to just add an unexported field to store the original/non-flattened reader, and use that in Outer? We already added another field for that very reason in #61870.

Basically, the way it could work is that in NewSectionReader we store all the original values (for use by Outer) and then we attempt to flatten the readers and store the flattened result in a second set of fields, used by all the other methods.

We could do that, with the obvious cost of making the structure fatter for a relatively niche semantic difference. If we're forced to maintain compatibility, that's certainly an option, but I think I would advocate for adjusting the semantics of Outer to make the more efficient internals possible.

(That is, assuming I'm right that Outer hasn't shipped in a stable release yet. If it has, then we have to do what you propose)

For my particular use case in Matroska parsing, I think the two options are broadly equivalent because I expect the intermediate SectionReaders to have approximately the same lifetime as the child SectionReaders, so it's all about reducing processing cost rather than memory cost. That said, for other parsing use cases, it may be nice to not force the pinning of intermediate readers, so that the programmer is free to optimize for readability, even if it means throwing around a few more SectionReaders during the parse.