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

encoding/gob: better support for error #23340

Open
MBonell opened this issue Jan 4, 2018 · 14 comments

Comments

Projects
None yet
6 participants
@MBonell
Copy link

commented Jan 4, 2018

Errors created by the errors package are not registered with encoding/gob which is necessary in order to send them as results of remote procedure calls.

By now, returning an error from the standard library through an RPC method will cause that the receiver get an EOF error and the transmitter a gob: type not registered for interface: errors.errorString

My proposal is add an init() function in the errors package to register the struct errorString so then, developers will avoid create its own errors implementation when using RPCs.

@bradfitz bradfitz changed the title Proposal for errors/errors: Support for gob-registered errors proposal: encoding/gob: Support for gob-registered errors Jan 4, 2018

@gopherbot gopherbot added this to the Proposal milestone Jan 4, 2018

@gopherbot gopherbot added the Proposal label Jan 4, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

Can you show some code demonstrating where this is needed? Thanks.

@MBonell

This comment has been minimized.

Copy link
Author

commented Jan 5, 2018

Here: https://golang.org/src/errors/errors.go

We just need to add the init function to register the error struct type.
E.g: https://play.golang.org/p/GAVhHG0m10W

@MBonell MBonell closed this Jan 5, 2018

@MBonell MBonell reopened this Jan 5, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

Thanks, but I'm not asking to see the change you are proposing. I'm asking to see an example of a program that will be fixed by this change.

@MBonell

This comment has been minimized.

Copy link
Author

commented Jan 5, 2018

@ianlancetaylor Sure, I've created this sample code to showcase the use case where you want to return an error in a response struct but you get the EOF error (client) and the rpc: gob error encoding body: gob: type not registered for interface: errors.errorString (server) errors.

https://github.com/MBonell/simple-rpc-server

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

Thanks. That was much more complicated than I was expecting, and I can't run it to see the problem, but it does suggest this program:

package main

import (
	"bytes"
	"encoding/gob"
	"errors"
	"log"
)

type S struct {
	Err error
}

func main() {
	var buf bytes.Buffer
	enc := gob.NewEncoder(&buf)
	dec := gob.NewDecoder(&buf)
	err := enc.Encode(S{errors.New("error")})
	if err != nil {
		log.Fatal(err)
	}
	var s S
	err = dec.Decode(&s)
	if err != nil {
		log.Fatal(err)
	}
}

Running that program fails with

2018/01/05 15:28:02 gob: type not registered for interface: errors.errorString

which I take to be the kind of error you are discussing.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 5, 2018

Change https://golang.org/cl/86456 mentions this issue: errors, encoding/gob: support gob transmission of errors.New

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jan 5, 2018

I think that if we decide to do this, the right approach is https://golang.org/cl/86456. That approach avoids importing encoding/gob in the errors package. We do not want the low level errors package to depend on the high level encoding/gob package.

@MBonell

This comment has been minimized.

Copy link
Author

commented Jan 5, 2018

Yes, your example is more simple to reproduce this issue because mine is the entire use case for RPC.
Agree with your implementation to avoid the encoding import.

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

FWIW, if we need to fix this in package errors, using encoding.BinaryMarshaler or encoding.TextMarshaler would be more generally applicable than the gob-specific interfaces (and will be picked up by gob).

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

/cc @robpike

@robpike robpike self-assigned this Jan 9, 2018

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2018

Rather than teach gob about one particular way of making errors, a better plan might be to have gob implement a fallback for errors in general. If something implements the error interface, gob could arrange to transmit the string and recover the error value on the other side, automatically. For those who want richer, type-aware semantics for errors, for example as is done in https://github.com/upspin/upspin/tree/master/errors, one can always implement one of the standard marshaling interfaces for that type, which would override the default.

I've been thinking about doing this for a while; perhaps the time has come to do something.

@rsc rsc changed the title proposal: encoding/gob: Support for gob-registered errors encoding/gob: better support for error Jan 22, 2018

@rsc rsc modified the milestones: Proposal, Go1.11 Jan 22, 2018

@rsc

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2018

@robpike will look into doing something for Go 1.11.

@robpike robpike modified the milestones: Go1.11, Go1.12 May 10, 2018

@danmux

This comment has been minimized.

Copy link

commented Jun 15, 2018

fwiw, I'm not convinced it is needed. The loss of type will still surprise some, but a little more insidiously. The person who expects the real type after unmarshaling will get caught out... quietly.

Marshaled structs typically form part of a contract that crosses process boundaries, and these contracts should attempt to remain simple and or explicit. In the simple case I would rather see a string type in the contract and it populated by an err.Error() so as to avoid the looming disappointment. If more complex errors are to form part of the contract then I agree the explicit marshaller interface as per @robpike s upspin example is the better approach.

I would say the marshalling of errors is almost binary:

  1. The client of the unmarshaled error expects an error type, and if they do they will surely have, or soon have, more complex needs like wrapped errors (a.la. upspin)
  2. The error may as well be explicitly a string

In case anecdotes are in any way useful. In 4 years of working with rpc contracts across 40 services this issue has come up once, and never wrt persistence (we do gob encode for persistence). We found it quickly because it failed loudly.

@robpike

This comment has been minimized.

Copy link
Contributor

commented Jun 15, 2018

In the model I have, the special handling would occur only if the implementing type is not otherwise marshalable, which I think addresses your worry.

@lieut-data lieut-data referenced this issue Sep 13, 2018

Merged

MM-11734: better plugin `error` handling #9405

1 of 1 task complete

@robpike robpike removed this from the Go1.12 milestone Nov 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.