BufReader should provide a way to seek without dumping the buffer

[taralx]

BufReader's seek() implementation always dumps the buffer. This is not good for performance, where users may want to skip a variable amount of buffered data -- only BufReader really knows if it's reasonable to move the pointer or not.

Additionally, only BufReader can know if the pointer can be reversed or not -- so there's absolutely no cheap way to "unget" data from BufReader, even if you know it's seekable.

I'd recommend removing this behavior and moving it to unwrap() or another method (sync?), but it's now baked into a stable API.

What are the options now?

[taralx]

(I'm in the process of writing my own BufReader implementation that doesn't do this.)

[neokril]

Currently BufReader saves only the offset within buffered data.
As I understand to keep buffer during seek it needs to save "global offset" within inner Reader and update it in all read/seek operations.
It needs this "global offset" because seek operation should return new position in underlying Reader which is not currently saved anywhere.

Another variant is to make a separate function (e.g. buffered_seek) which will use only buffered data, will move only relative to current position and return true/false (true if it was able to move current position inside the buffer; false - nothing happened and user needs to make a separate call to "seek" to actually move the pointer).

[taralx]

I'm not trying to avoid seeking the underlying stream, I'm trying to avoid dumping the buffer. In my implementation, small seeks that fit in the existing buffer cause a call to self.inner.seek(SeekFrom::Current(0)).

[neokril]

@taralx Could you please share your implementation?

You use self.inner.seek(SeekFrom::Current(0)) to get current file offset. Am I correct?

[taralx]

Not without getting source approval from my employer, sorry.

Yes, I implement pub fn pos(&mut self) -> io::Result<u64> and then in seek I have snippets like:

let p = try!(self.pos());
self.pos -= back;
return p - back;

[gcarq]

Some time ago I had a similiar requirement, I extracted the implementation into a own library: seek_bufread, maybe its useful for you.

[lolgesten]

I stumbled upon this today. When decoding mp4 movies, the decoder will do small seeks forward to skip in the region of 100 bytes at a time. I used a BufReader with 65k capacity to avoid my underlying reader doing http requests for every seek, only to find that BufReader isn't helping at all.

I think BufReader could be more useful if it worked from the buffer also when seek fits.

[dtolnay]

I would like to see a PR that implements seek(SeekFrom::Current(n)) without dumping the buffer if the offset n is within the buffered data. I may be missing something but I don't think BufReader would need to store a global offset in order to make this work.

[taralx]

FWIW, this solution requires R:Seek. A try_seek_relative function would not have such a requirement.

[SimonSapin]

This should not be closed, the new method is still unstable. It’s been a couple months though:

@rfcbot fcp merge

[alexcrichton]

@SimonSapin to confirm, this is for the seek_relative method, right?

[SimonSapin]

Yes, that is the one item with issue = "31100".

[SimonSapin]

Hmm, maybe I needed to reopen the issue before making the command:

@rfcbot fcp merge

[rfcbot]

Team member @SimonSapin has proposed to merge this. The next step is review by the rest of the tagged teams:

• @Kimundi
• @SimonSapin
• @alexcrichton
• @aturon
• @dtolnay
• @sfackler
• @withoutboats

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period is now complete.

[taralx]

No, the major issue is one of compatibility guarantees. Right now, if you want to sync the BufReader's position with the underlying Reader and extract it, you seek and then into_inner. So that pattern needs to continue to work.

[funchal]

Can't we just leave seek alone then and add a separate api seek2 or whatever which has the little enhancement of not losing the buffer when you seek within it?

[sfackler]

That already happened: https://doc.rust-lang.org/std/io/struct.BufReader.html#method.seek_relative

[taralx]

I've been advocating for try_seek_relative because the existing method requires R: Seek where that's not necessary for a BufReader.

[czipperz]

@taralx do you want to write up a pr? I doubt it would be very difficult. Just refactor seek_relatives code that already does this out into the method?

[czipperz]

I do find it mildly ironic that the original implementation explicitly documented that it would always flush the buffer and why.

[taralx]

Life is a little busy right now, so if someone else is excited to do it, go for it. Otherwise I'll see if I can't find some time to put together a PR.

[fintelia]

@SimonSapin it seems that there was a final comment period for this in Mar 2018 that passed with no objections. Does that now mean there can be a stabilization PR?

[t-rapp]

At least for my use-case of parsing media files I'm still relying on the seek_bufread crate instead of using seek_relative because there seems to be no way in stdlib BufReader to get the current position without dumping the buffer.

[maxburke]

One of the challenges I have been finding with BufReader is that the stream_position convenience method is implemented with seek(SeekFrom::Current(0)) which dumps the buffer.

Is it possible to have this case specialized so that getting the current position doesn't cause the buffer to be thrown away?

[alecmocatta]

I'm not entirely clear on the status of BufReader::seek_relative. It was implemented, this issue was denoted as the tracking issue, there's was a successful fcp, but it's still unstable? What was the fcp for?

[Diggsey]

@alecmocatta someone still needs to open a PR to mark the method as stable.

[elomatreb]

As someone unfamiliar with the processes of Rust development, is there a chance of getting this into one of the upcoming releases?

[workingjubilee]

The relevant PR passed FCP, has been approved to merge, and this should become stable in 1.53.0 if all goes well.