Skip to content

Remove IO.read_exactly#361

Merged
rgrinberg merged 1 commit into
mirage:masterfrom
rgrinberg:remove_read_exactly
Jun 2, 2015
Merged

Remove IO.read_exactly#361
rgrinberg merged 1 commit into
mirage:masterfrom
rgrinberg:remove_read_exactly

Conversation

@rgrinberg
Copy link
Copy Markdown
Member

It's not being used anywhere as of #360.

Are there any good cases for keeping it? The main problem with this function
was how it sneaked in allocations of very large buffers into cohttp's code
base.

We could of course fix it by making sure it checks its argument and raises when
its above the maximum threshold. But in that case the user is just better off
using read. Since he will have to handle buffers larger than read_exactly
can handle using read anyway

@vbmithr Rather than fixing this function by raising when it try to raise large
buffers we can simply get rid of it.

cc @avsm @samoht

PS I don't consider this as a breaking change because IO is not officially part of the interface we expose.

It's not being used anywhere as of mirage#360
@vbmithr
Copy link
Copy Markdown
Member

vbmithr commented May 30, 2015

Yeah, I would remove it. It would be nice to have a function that reads into a buffer instead of allocating, that would be more useful than this.

@vbmithr
Copy link
Copy Markdown
Member

vbmithr commented May 30, 2015

Actually, I would personally delete all IO functions that allocate buffers themselves. Reader has AFAIKS no one of them (apart from read_char, read_line, etc.). I don't like Lwt_io.read either.

And overall I'm 100% for deleting dead code, so I would say, go ahead :)

@rgrinberg
Copy link
Copy Markdown
Member Author

I agree with that as well but just note that it can't be done very easily with current design. The problem is that we buffer things like request/response bodies. Unfortunately, this means that we must allocate more than one buffer for some requests. Changing Lwt_io.read into something like Lwt_io.read_into would simply move the buffer allocation from one place to another.

@vbmithr
Copy link
Copy Markdown
Member

vbmithr commented May 30, 2015

No but nevertheless it is more flexible to have functions that let you handle the allocation instead that function that allocate things for you.

@samoht
Copy link
Copy Markdown
Member

samoht commented Jun 2, 2015

fine with me

rgrinberg added a commit that referenced this pull request Jun 2, 2015
@rgrinberg rgrinberg merged commit b76fba4 into mirage:master Jun 2, 2015
@rgrinberg rgrinberg deleted the remove_read_exactly branch June 2, 2015 15:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants