Hacker News new | ask | show | jobs
by ericbarrett 1318 days ago
I write a lot of Go, and I agree that this is a big wart in its error handling that would be served by a proper Result type.

Nevertheless, the convention is that if a function returns (value, err), and err != nil, the value is discarded (I think of it as "undefined"). So the code is conventional.

2 comments

In C, “discarding” a pointer in a way that leaves the value visible is quite common. At least if one doesn’t accidentally use the pointer, it’s harmless. (In the way that all manner of unsafeness is harmless in C as long as no actual UB occurs, which is to say it’s not great.)

But Go is a garbage collected language, and there is so such thing as “discarding” a pointer. Either it’s there or it isn’t, and this kind of leak has side effects. I find it baffling that the language designers and the community consider this acceptable.

(One thing I really like about Rust is that you can’t play fast and loose with lifetimes like this. If you have function taking &'a Vec<T> and you return &'a T, you can’t arbitrarily “discard” and leak that reference up the call chain. You need to genuinely get rid of it by the time the vector is gone.)

>and this kind of leak has side effects

Only if the calling function does something with the pointer (which it generally won't, if err is non-nil). If the calling code does do something with the pointer even when err is non-nil, then either

(a) the value of the pointer is meaningful, and this is fine; or

(b) there's a logic error in the calling code, which is a far more serious bug than a potential memory leak.

So I don't really see the problem here.

I should have been more explicit. Go is garbage collected. If you keep a pointer live, then it’s not garbage, which prevents it from being collected.

At least Go doesn’t use RAII, so inadvertently pinning a pointer won’t keep a socket open or a lock held.

Just returning the pointer doesn't keep the value that it points to live, unless the calling function does something with the pointer.

The following toy example doesn't leak memory. Are you thinking of some other pattern that would leak memory? If so, what is it? I can only think of patterns where a logic error is also involved. That is, where the meaningless value of 'val' is used in code paths where err != nil.

    func alwaysError() (*int, error) {
        var dummy int

        // We return &dummy even though it's a meaningless value.
        // Will this cause a memory leak?

        return &dummy, fmt.Errorf("An error")
    }

    func caller() {
        val, err := alwaysError()
        if err != nil {
            fmt.Printf("Error\n")

            // It won't! Because the value pointed to by 'val'
            // can be GCed from this point on.
            return
        }

        // never get here        

        fmt.Printf("Value %v\n", val)
        //
        // ...lots of code that uses *val...
        //
    }
You might think that you'd get a leak if the error branch was also long-lived, but Go's GC seems to be precise with respect to conditional branching (as far as I can tell by experimenting with runtime.SetFinalizer). That is, as long as the error branch doesn't refer to 'val', then the value pointed to by 'val' can be collected once the error branch is entered (and before 'caller' returns).
I haven't actually played with this, but if the same pattern of lazily (sloppily?) returning arbitrary pointers along with errors continues, the lifetime ought to be extended arbitrarily. The caller could itself return val, err, and so on up the chain.
I'm not seeing how that would cause a memory leak. It just means that the value might be GCed a bit later. Could you give an example?
What I found frustrating that there is nothing stopping a function from returning both a non nil value and a non nil error. I've seen weird code that did that and it could have easily resulted in incorrect behavior.
The one place where you’ll actually see this is in Read().

If you try to read 1000 bytes, you might get “800 bytes read, EOF” as a result. The worst part is that os.File won’t ever do this!

I really hate when people use "errors" for signals. Or, alternately, use the signals mechanisms for actual errors. An error should be "something went wrong", and reading a file to the EOF is not "going wrong", that's just what the "read()" should do if you tell it so!

I like Go's multiple returns and error checking by default, but it definitely should have been implemented with some sort of "Result/Error" type union instead. Go made so many good decisions, that I am frankly amazed they made this one really bad one.

If you take the position that the semantics of read() are 'read data from this endless stream', then EOF is an error indicating that this model no longer applies.

Doesn't bother me at all.

Except in that model it would be fine to return an error as soon as soon as you know there is an EOF while in the real world you do want to read right up to the EOF because EOF is usually not an error condition but expected, even with streams of indeterminate length.
The model is that you don’t know there’s an EOF until after you’ve read the last data byte.
But that's what "errors" are, the're signals.
Presumably this is for performance reasons: if a `Read` call involves, say, a network request, then returning "800, EOF" in one roundtrip is certainly nicer than "800, nil" -> "0, EOF" in two roundtrips. In practice... I suspect this happens rarely enough that it's not worth the cost. The performance edge case could be handled with an alternative API, e.g. an optional EOF() method.