dedis / protobuf

Reflection-based Protocol Buffers for Go
GNU General Public License v2.0
76 stars 15 forks source link

Convert panic to error #46

Closed ineiti closed 6 years ago

ineiti commented 6 years ago

Most of the error handling in protobuf is done using a panic that is handled in a recover action. This should be converted to proper error-returning of the methods. Even if it means to add all those pesky

if err != nil{
  return nil, err
}
jeffallen commented 6 years ago

It is a legitimate pattern to use panic inside of a library, and recover at the edge of the library to return an error to the caller. Thus, I see nothing to fix in encode.go.

I see two panics in decode.go that would escape to the caller, so those need to be fixed.

There is also a panic that can escape from GenerateProtobufDefinition, but it is not a critical part of the API.

ineiti commented 6 years ago

OK - so continue using panic and make sure that all panics get correctly caught before returning to the caller.

aounleonardo commented 6 years ago

The two panics in decode.go can be occured by putvalue and instantiate. Just pushed a fix that catches the panics thrown by instantiate in putvalue. And I saw that all panics thrown by putvalue are directly being caught like in:

if err := de.putvalue(wiretype, val, v, vb); err != nil {
        return nil, err
    }

I also tried catching the panic in GenerateProtobufDefinition but am not sure if what I did is indeed correct.