Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bufio: add function to read a certain amount of bytes #17064

Closed
n10v opened this issue Sep 11, 2016 · 10 comments
Closed

bufio: add function to read a certain amount of bytes #17064

n10v opened this issue Sep 11, 2016 · 10 comments

Comments

@n10v
Copy link
Contributor

n10v commented Sep 11, 2016

In many programs, where I use bufio package, lacks the function, that can read certain amount of bytes.
So now I should create function like this:

func ReadSeveralBytes(r *bufio.Reader, n int) ([]byte, error) {
    if n == 0 || r == nil {
        return nil, nil
    }

    peeked, err := r.Peek(n)
    if err != nil {
        return nil, err
    }

    if _, err := r.Discard(n); err != nil {
        return nil, err
    }

    return peeked, nil
}

But it's inconvenient and I think, the way it works could be better.
I hope, what I'm not the only one, who wants it in bufio package.

@Thomasdezeeuw
Copy link
Contributor

Take a look at https://golang.org/pkg/io/#ReadFull. I think that is what you want. Although I'm not sure why you would use Peek and then Discard.

@n10v
Copy link
Contributor Author

n10v commented Sep 11, 2016

Hm, I didn't see this function earlier, but it's closer to what I want, but not the perfect.
I should always allocate new buffer. It consumes more memory than just the way Peek function works.

@uluyol
Copy link
Contributor

uluyol commented Sep 11, 2016

Doesn't

b := make([]byte, n)
got, err := r.Read(b)

do exactly this?

@bradfitz
Copy link
Contributor

@uluyol, that allocates, and the io.Reader contract doesn't guarantee to read the requested number of bytes. Peek will load them in.

@n10v
Copy link
Contributor Author

n10v commented Sep 11, 2016

Look, I have this function in my project:

// ReadSeveralBytes reads n bytes.
func (r *Reader) ReadSeveralBytes(n int) ([]byte, error) {
    if n == 0 {
        return nil, nil
    }

    peeked, err := r.buf.Peek(n)
    if err != nil {
        return nil, err
    }

    if _, err := r.buf.Discard(n); err != nil {
        return nil, err
    }

    return peeked, nil
}

If it will allocate a slice on every call of function, it will consume a lot of memory. But Peek just returns the slice of already allocated array, so it consumes noticeably less memory, what is very important in my project.

@dsnet
Copy link
Member

dsnet commented Sep 12, 2016

At least the naming of ReadSeveralBytes seems off to me since it makes it seem like I can pass in an integer like 1<<20 and it would return me a 1MiB byte slice. The documentation of Peek is clear that the method fails if the requested size is greater than the internal buffer. This is a subtle detail and I think the number of methods that bufio has with subtle conditions attached should be limited.

@bradfitz
Copy link
Contributor

I agree. I'm going to close this issue. I'm happy we got bufio.Discard added because it enables helpers like this one, but I don't think bufio needs it itself. It's useful sometimes, but not enough to add to the docs and overwhelm most users with the cognitive overload of many too-similar functions.

@n10v
Copy link
Contributor Author

n10v commented Sep 12, 2016

@bradfitz So, when I want to read several bytes, what should I use? Peek + Discard?
And I don't think, what library will be overwhelmed with this simple function, whose purpose is very clear. I think, what this function will be a good and logic addition to bufio.
I am sure, that I'm not the only one, who needs similar method in standard library. And it's not so similar as other functions. Peek just peeks some chunk of memory, but doesn't advance the reader. Discard just advances the reader and doesn't return any memory slice. And this function will read several bytes and advance buffers. It's very easy to understand and I'm sure, what it won't overwhelm bufio library.
@dsnet Yes, I agree, what the naming isn't good. We should think about a good name.
I think, it should be noticed in documentation for this method, what the method fails if the requested size is greater than the internal buffer as you said and what user should always check the returned byte slice and error.

@bradfitz
Copy link
Contributor

So, when I want to read several bytes, what should I use? Peek + Discard?

Just using Read is the most clear. But that requires a copy and you have to have a buffer to read into (or for you to allocate). If you need the performance and don't have a buffer handy, then use Peek + Discard.

And I don't think, what library will be overwhelmed with this simple function, whose purpose is very clear. I think, what this function will be a good and logic addition to bufio.

Users who need this already can do it, using the function you already wrote. I don't think it needs to be part of the standard library. I think it would do more harm (cognitive overload, documentation clutter) than good (benefiting a few users, reducing their need to write a small function).

@n10v
Copy link
Contributor Author

n10v commented Sep 12, 2016

Ok, I agree with you.
There are already many ways to do it:

  1. Read
  2. io.ReadFull
  3. Peek + Discard

And it will overwhelm library. Now I understood this. Thanks for explanation and sorry for wasting your time!

@golang golang locked and limited conversation to collaborators Sep 12, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants