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.

Sign up for GitHub

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

Jump to bottom

proposal: bytes: don't panic ErrTooLarge / remove it / refactor bytes.Buffer API #52599

Closed
Antonboom opened this issue Apr 28, 2022 · 3 comments
Closed

proposal: bytes: don't panic ErrTooLarge / remove it / refactor bytes.Buffer API #52599

Antonboom opened this issue Apr 28, 2022 · 3 comments
Labels
FrozenDueToAge Proposal
Milestone
Proposal

Comments

@Antonboom
Copy link

Antonboom commented Apr 28, 2022 •
edited

Hello!

Since Go 1.16 in std no code that uses bytes.ErrTooLarge.

Panicking with sentinel error is a bad example given the standard library to Go's newcomers. It leads to logic around of panic-recover instead of using error flow and errors.Is.

I ask you to pay attention to this.

History of changes:

  1. @robpike introduced bytes.ErrTooLarge as sentinel error with error flow (link).
  2. @robpike supported ioutil.ReadAll and introduced panicking (link).
  3. @bcmills continued panic practice according to proposal (link).
  4. @rsc rewrote ioutil.ReadAll and moved it in io (link). Now in io no dependency to bytes.ErrTooLarge.

P.S.

This code

func makeSlice(n int) []byte {
    // If the make fails, give a known error.
    defer func() {
        if recover() != nil {
            panic(ErrTooLarge)
        }
    }()
    return make([]byte, n)
}

can be rewrited as

func makeSlice(n int) (s []byte, err error) {
    defer func() {
        if recover() != nil {
            err = ErrTooLarge
        }
    }()
    return make([]byte, n), nil
}

But I have no idea about:

} else if c > maxInt-c-n {
    panic(ErrTooLarge)
}
@gopherbot gopherbot added this to the Proposal milestone Apr 28, 2022
@bcmills
Copy link
Contributor

bcmills commented Apr 28, 2022

@bcmills continued panic practice according to #19624 (link).

In general a very large (*bytes.Buffer).Grow can fail in one of two ways: it can run the process out of memory (which causes an unrecoverable runtime throw), or it can fail to even compute a sensible size (which is what results in the aforementioned panic.

Both of those outcomes result from programmer errors, and should be avoidable through sensible bounds-checks on the caller side. So I don't see any benefit to removing the existing panic.

@Antonboom
Copy link
Author

@bcmills I have nothing against panic in this case, but I'm confused by panic with public error.
What logic is expected for this error on the package client's side?

@ianlancetaylor
Copy link
Contributor

The proposed change is reasonable but it's not backward compatible. I think that making this change would violate the Go 1 compatibility guarantee.

I'm going to close this issue. Please comment if you disagree.

To answer your question, the client side is not expected to create a buffer so large that it can't be represented. For buffers that can grow without bounds, add a bounds check. Code is not expected to catch this panic, it's expected to avoid the panicking case.

@golang golang locked and limited conversation to collaborators Apr 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Proposal
Projects
None yet
Milestone
Proposal
Development

No branches or pull requests
4 participants
@ianlancetaylor @bcmills @gopherbot @Antonboom