|Anonymous | Login | Signup for a new account||2019-02-24 05:43 CET|
|Main | My View | View Issues | Change Log | Roadmap|
|View Issue Details|
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0006719||OCaml||standard library||public||2014-12-16 22:07||2016-12-07 11:47|
|Target Version||Fixed in Version||4.03.0+dev / +beta1|
|Summary||0006719: Buffer.add_channel is unusable|
|Description||Buffer.add_channel behavior is flawed. In case the channel contains less than [n] bytes (where [n] is the last parameter to [Buffer.add_channel buf chan n]), some data is consumed but not read, and lost forever if the channel isn't seekable.|
This makes it almost unusable in my opinion (and it looks like it's not used in trunk, as "grep -r Buffer.add_channel" shows).
|Steps To Reproduce||1/ echo "hello" > /tmp/some_file|
let buf = Buffer.create 256 in
let ic = open_in "/tmp/some_file" in
(try Buffer.add_channel buf ic 256 with End_of_file -> ());
We obtain an empty string. In practice, unless we need how much bytes to read exactly beforehand, we just consume data from the in_channel and discard it. It makes writing a simple procedure to read all data from a channel into a Buffer impossible (except by reading char by char).
I'd say Buffer.add_channel isn't flawed because it raises an exception for a short read. There are numerous protocols where the number of bytes are prepended to payload data, and add_channel works fine for these. A short read is always an error in this case.
The problem reported by c-cube sounds more like a feature request for a second channel reading function that also appends short reads to the buffer.
edited on: 2014-12-17 22:45
I'd say it's a good feature request. Discarding data is almost never the right thing to do.
Here is what I propose: change the documentation to say "reads at least [n] characters" instead of "reads exactly [n] characters", and change the implementation to put the bytes in the buffer instead of discarding them. But still raise End_of_file if there aren't enough bytes.
|sounds ok. I'll submit a PR.|
|How would you identify the bytes making up the partial request? It may be necessary to specially handle an incomplete read.|
Another option would be to add
that does what c-cube wants in the first place.
It can be done *instead* of changing add_channel, or in conjunction with it.
hcarty: if you know you need this, it's possible to workaround the fact that add_channel cannot return the number of read bytes by storing the length of the buffer before and after the request, and computing the difference.
What I like with Damien's suggestion is that it strikes a subtle balance between preserving backward-compatibility (the behavior is *almost* unchanged) while satisfying a new use-case. I agree (with hcarty) that if we were doing it from scratch, we would probably have a different interface¹, but there is an art in evolving APIs that is admirable here.
¹: namely, "add_channel" that returns the number of bytes read, and raises End_of_file only if it knows the channel ends, and "add_channel_really" with the current interface.
|gasche: That's true - I was hoping for something cleaner, but as you said there is an art to API evolution.|
|FTR I wrote "at least" when I meant "at most". You did it right in the GPR (https://github.com/ocaml/ocaml/pull/129 [^]).|
|Fixed using Damien's proposed behavior, thanks to c-cube's patch.|
|2014-12-16 22:07||c-cube||New Issue|
|2014-12-17 15:39||gerd||Note Added: 0012835|
|2014-12-17 22:03||doligez||Note Added: 0012837|
|2014-12-17 22:43||doligez||Severity||minor => feature|
|2014-12-17 22:43||doligez||Status||new => confirmed|
|2014-12-17 22:45||doligez||Note Edited: 0012837||View Revisions|
|2014-12-17 22:46||doligez||Tag Attached: junior_job|
|2014-12-18 00:45||c-cube||Note Added: 0012847|
|2014-12-18 01:07||hcarty||Note Added: 0012848|
|2014-12-18 09:30||gasche||Note Added: 0012850|
|2014-12-18 16:32||hcarty||Note Added: 0012866|
|2014-12-18 17:48||doligez||Note Added: 0012871|
|2015-04-12 13:11||gasche||Note Added: 0013671|
|2015-04-12 13:11||gasche||Status||confirmed => resolved|
|2015-04-12 13:11||gasche||Fixed in Version||=> 4.03.0+dev / +beta1|
|2015-04-12 13:11||gasche||Resolution||open => fixed|
|2015-04-12 13:11||gasche||Assigned To||=> gasche|
|2016-02-05 04:43||gasche||Relationship added||has duplicate 0007136|
|2016-12-07 11:47||xleroy||Status||resolved => closed|
|2017-02-23 16:43||doligez||Category||OCaml standard library => standard library|
|Copyright © 2000 - 2011 MantisBT Group|