BufRead: Only flush the internal buffer if seeking outside of it.

[Diggsey]

Fixes #31100

r? @dtolnay

[Diggsey]

Actually, don't merge this: there's an issue with the behaviour when seeking within a buffer and then reading past the end.

@dtolnay to fix this, I'd need to store an extra "offset" field, and then when refilling the buffer, it will have to perform an extra seek to get back to the right position if the offset is non-zero.

[Diggsey]

So I think I'm going to need some input on this: the original code has a comment explaining that BufReader guarantees that if you call seek() followed immediately by unwrap(), the underlying reader will be at that point.

There doesn't seem to be any justification for this guarantee, and it is not clear whether this should apply to get_mut(), get_ref(), etc. which also allow access to the inner reader. If we can just drop the guarantee, the implementation becomes very straightforward. If not, there are a variety of trade-offs to consider.

@kballard it looks like you wrote the original implementation here - can you provide any input?

[dtolnay]

I would have preferred a separate method specifically for seeking the underlying reader to self.pos, but at this point I believe we need to keep the guarantee about seek followed by into_inner. What are the trade-offs to consider?

[Diggsey]

To start with, all BufRead<T: Seek> will need to store a corrective offset to apply. The question is then when the offset is applied:

1. When a seek is performed within the buffer, no seek instruction is passed to the inner reader: the offset is stored, and applied only in into_inner - in this case, while the original guarantee technically still holds, accessing the inner reader via get_mut() or get_ref() will not see the reader at the seek location.
2. The offset is applied before performing any further reads from the inner buffer - in this case, the original guarantee holds, and also get_mut() and get_ref() will see the buffer at the seek location. However, these seeks will be completely redundant, and seek must be called a second time on the inner reader when it must next be read from, and these two extra seeks may eliminate any benefit to this optimisation in the first place.

Option 1 makes more sense to me, except that the original guarantee is not super useful if it only applies to into_inner: why not just seek after unwrapping the reader? It seems more likely that if anyone is relying on this at all, they would be relying on the implicit suggestion that the underlying reader be at the seek position immediately after a seek is performed, regardless of how that reader is accessed. If this assumption is being broken by implementing option 1, it would make more sense to go with option 0 and just drop the guarantee entirely.

[dtolnay]

Handing this off for a fresh set of eyes, but I think reasonable next steps would be:

• Agree on what aspects of the behavior are acceptable to be changed -- in particular around get_ref and get_mut seeing the seeked location but also any other observable consequences of BufReader should provide a way to seek without dumping the buffer #31100.

• Track down some use cases that involve seeking a BufReader in real code and understand how they would be impacted by seeks that do not dump the buffer.

[sfackler]

@alexcrichton Do you remember why the original behavior was used?

[dtolnay]

Dumping the buffer on every seek, and guaranteeing that into_inner sees the underlying reader at the seeked position? Doesn't look like there was any discussion about that in #24176.

[alexcrichton]

Oh dear unfortunately I have no recollection of adding this beyond rubber-stamping it. I don't recall any particular discussion about this particular point.

After trying to understand this for 20 minutes I also had to give up myself. I'm not familiar enough with everything at this point in time to give a recommendation myself.

[carols10cents]

triage ping for you @sfackler!

[alexcrichton]

The @rust-lang/libs team discussed this PR during triage today and the conclusion was that we probably can't change the behavior here (as it's explicitly documented as not doing this), but would it be a possibility to perhaps add a new inherent method and mention it in the documentation?

[Diggsey]

@alexcrichton I've updated the PR accordingly.

[alexcrichton]

@bors: r+

[bors]

📌 Commit c96f302 has been approved by alexcrichton

[bors]

⌛ Testing commit c96f302 with merge 80e2e67...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 80e2e67 to master...