fix sendfile bugs with buffered reader contents #24905
Conversation
seekBy affects the reader's physical position, not its logical position.
Additionally handles already buffered contents.
| @@ -1853,7 +1853,13 @@ pub const Writer = struct { | |||
| return error.EndOfStream; | |||
| } | |||
| const consumed = io_w.consume(@intCast(sbytes)); | |||
| file_reader.seekTo(file_reader.pos + consumed) catch return error.ReadFailed; | |||
| if (consumed <= file_reader.interface.bufferedLen()) { | |||
There was a problem hiding this comment.
Would you mind fixing this confusing variable name while you are here? The return value of consume() is not the number of bytes "consumed" but rather the number left over after consuming bytes in the writer's buffer.
Perhaps remainder would be better? I'm having trouble thinking of other alternatives...
| if (consumed <= file_reader.interface.bufferedLen()) { | ||
| file_reader.interface.toss(consumed); | ||
| } else { | ||
| const directn = consumed - file_reader.interface.bufferedLen(); |
There was a problem hiding this comment.
I'm not a huge fan of directn either (though it is better than consumed). What do you think of to_seek here?
There was a problem hiding this comment.
I think directn is pretty clear, it is the number of bytes directly transferred from the reader to writer (i.e. without going through the reader).
to_seek could work, but I feel it doesn't explain as clearly what the reader is being seeked by.
There was a problem hiding this comment.
Maybe it's just my gut reaction to variables without word separation. I like both direct and direct_bytes or even direct_n more than directn.
| const n = try file_reader.read(dest); | ||
| w.advance(n); | ||
| return n; | ||
| return file_reader.interface.stream(w, limit); |
There was a problem hiding this comment.
I don't think this is OK, stream may be implemented as a call to sendFile while this function must work even if sendFile is not supported.
It would be good to add a comment here explaining this.
There was a problem hiding this comment.
I believe the same problem exists with the other implementation. For example, File.Reader could use defaultReadVec which constructs its own temporary Writer and calls stream on. If File.Reader were to then use sendFileAll on it, it would end up back at this case.
However, I think the premise here is the File.Reader is implemented in such a manner that this will not occur. It could be added to the doc comments of sendFileReading and friends that they must not be called from File.Reader, though I am unsure that is necessary.
There was a problem hiding this comment.
I believe the same problem exists with the other implementation. For example, File.Reader could use defaultReadVec which constructs its own temporary Writer and calls stream on. If File.Reader were to then use sendFileAll on it, it would end up back at this case.
No, this problem does not exist with the other implementation. This problem cannot exist if the writer on which sendFileReading() was called is not exposed to file_reader here. The existing implementation only exposes the buffer to file_reader.
There was a problem hiding this comment.
My point is that it is not so much the writer being passed (the reader implementation should be agnostic to this), but instead its behavior. In both cases, the passed writer to file_reader will result in error.Unimplemented. Therefore, the only difference here is that the new implementation uses vtable.stream while the old uses vtable.readVec (which, depending on the file_reader implementation, can desugar to vtable.stream with a writer that does not implement sendFile (e.g. defaultReadVec)).
(see commit messages for details)