Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006719OCamlstandard librarypublic2014-12-16 22:072016-12-07 11:47
Reporterc-cube 
Assigned Togasche 
PrioritynormalSeverityfeatureReproducibilityalways
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version4.03.0+dev / +beta1 
Summary0006719: Buffer.add_channel is unusable
DescriptionBuffer.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 Reproduce1/ echo "hello" > /tmp/some_file

2/ utop

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 -> ());
Buffer.contents buf


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).
Tagsjunior_job
Attached Files

- Relationships
has duplicate 0007136closedgasche Buffer.add_channel discards data 

-  Notes
(0012835)
gerd (reporter)
2014-12-17 15:39

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.
(0012837)
doligez (administrator)
2014-12-17 22:03
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.

(0012847)
c-cube (reporter)
2014-12-18 00:45

sounds ok. I'll submit a PR.
(0012848)
hcarty (reporter)
2014-12-18 01:07

How would you identify the bytes making up the partial request? It may be necessary to specially handle an incomplete read.
(0012850)
gasche (administrator)
2014-12-18 09:30

Another option would be to add

  add_channel_full

or

  add_full_channel

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.
(0012866)
hcarty (reporter)
2014-12-18 16:32

gasche: That's true - I was hoping for something cleaner, but as you said there is an art to API evolution.
(0012871)
doligez (administrator)
2014-12-18 17:48

FTR I wrote "at least" when I meant "at most". You did it right in the GPR (https://github.com/ocaml/ocaml/pull/129 [^]).
(0013671)
gasche (administrator)
2015-04-12 13:11

Fixed using Damien's proposed behavior, thanks to c-cube's patch.

- Issue History
Date Modified Username Field Change
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
Powered by Mantis Bugtracker