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

rpc: remove 'exported or builtin' restriction for parameters #20332

Merged
merged 2 commits into from
Nov 20, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions rpc/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,11 +40,11 @@ func TestClientRequest(t *testing.T) {
client := DialInProc(server)
defer client.Close()

var resp Result
if err := client.Call(&resp, "test_echo", "hello", 10, &Args{"world"}); err != nil {
var resp echoResult
if err := client.Call(&resp, "test_echo", "hello", 10, &echoArgs{"world"}); err != nil {
t.Fatal(err)
}
if !reflect.DeepEqual(resp, Result{"hello", 10, &Args{"world"}}) {
if !reflect.DeepEqual(resp, echoResult{"hello", 10, &echoArgs{"world"}}) {
t.Errorf("incorrect result %#v", resp)
}
}
Expand All @@ -58,13 +58,13 @@ func TestClientBatchRequest(t *testing.T) {
batch := []BatchElem{
{
Method: "test_echo",
Args: []interface{}{"hello", 10, &Args{"world"}},
Result: new(Result),
Args: []interface{}{"hello", 10, &echoArgs{"world"}},
Result: new(echoResult),
},
{
Method: "test_echo",
Args: []interface{}{"hello2", 11, &Args{"world"}},
Result: new(Result),
Args: []interface{}{"hello2", 11, &echoArgs{"world"}},
Result: new(echoResult),
},
{
Method: "no_such_method",
Expand All @@ -78,13 +78,13 @@ func TestClientBatchRequest(t *testing.T) {
wantResult := []BatchElem{
{
Method: "test_echo",
Args: []interface{}{"hello", 10, &Args{"world"}},
Result: &Result{"hello", 10, &Args{"world"}},
Args: []interface{}{"hello", 10, &echoArgs{"world"}},
Result: &echoResult{"hello", 10, &echoArgs{"world"}},
},
{
Method: "test_echo",
Args: []interface{}{"hello2", 11, &Args{"world"}},
Result: &Result{"hello2", 11, &Args{"world"}},
Args: []interface{}{"hello2", 11, &echoArgs{"world"}},
Result: &echoResult{"hello2", 11, &echoArgs{"world"}},
},
{
Method: "no_such_method",
Expand All @@ -104,7 +104,7 @@ func TestClientNotify(t *testing.T) {
client := DialInProc(server)
defer client.Close()

if err := client.Notify(context.Background(), "test_echo", "hello", 10, &Args{"world"}); err != nil {
if err := client.Notify(context.Background(), "test_echo", "hello", 10, &echoArgs{"world"}); err != nil {
t.Fatal(err)
}
}
Expand Down Expand Up @@ -393,9 +393,9 @@ func TestClientHTTP(t *testing.T) {

// Launch concurrent requests.
var (
results = make([]Result, 100)
results = make([]echoResult, 100)
errc = make(chan error)
wantResult = Result{"a", 1, new(Args)}
wantResult = echoResult{"a", 1, new(echoArgs)}
)
defer client.Close()
for i := range results {
Expand Down Expand Up @@ -450,7 +450,7 @@ func TestClientReconnect(t *testing.T) {
}

// Perform a call. This should work because the server is up.
var resp Result
var resp echoResult
if err := client.CallContext(ctx, &resp, "test_echo", "", 1, nil); err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -478,7 +478,7 @@ func TestClientReconnect(t *testing.T) {
for i := 0; i < cap(errors); i++ {
go func() {
<-start
var resp Result
var resp echoResult
errors <- client.CallContext(ctx, &resp, "test_echo", "", 3, nil)
}()
}
Expand Down
10 changes: 1 addition & 9 deletions rpc/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ Methods that satisfy the following criteria are made available for remote access

- method must be exported
- method returns 0, 1 (response or error) or 2 (response and error) values
- method argument(s) must be exported or builtin types
- method returned value(s) must be exported or builtin types

An example method:

Expand Down Expand Up @@ -74,13 +72,8 @@ An example server which uses the JSON codec:
calculator := new(CalculatorService)
server := NewServer()
server.RegisterName("calculator", calculator)

l, _ := net.ListenUnix("unix", &net.UnixAddr{Net: "unix", Name: "/tmp/calculator.sock"})
for {
c, _ := l.AcceptUnix()
codec := v2.NewJSONCodec(c)
go server.ServeCodec(codec, 0)
}
server.ServeListener(l)

Subscriptions

Expand All @@ -90,7 +83,6 @@ criteria:

- method must be exported
- first method argument type must be context.Context
- method argument(s) must be exported or builtin types
- method must have return types (rpc.Subscription, error)

An example method:
Expand Down
2 changes: 1 addition & 1 deletion rpc/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ func (h *handler) cancelAllRequests(err error, inflightReq *requestOp) {
}
for id, sub := range h.clientSubs {
delete(h.clientSubs, id)
sub.quitWithError(err, false)
sub.quitWithError(false, err)
}
}

Expand Down
8 changes: 0 additions & 8 deletions rpc/json.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,14 +153,6 @@ type ConnRemoteAddr interface {
RemoteAddr() string
}

// connWithRemoteAddr overrides the remote address of a connection.
type connWithRemoteAddr struct {
Conn
addr string
}

func (c connWithRemoteAddr) RemoteAddr() string { return c.addr }

// jsonCodec reads and writes JSON-RPC messages to the underlying connection. It also has
// support for parsing arguments and serializing (result) objects.
type jsonCodec struct {
Expand Down
28 changes: 2 additions & 26 deletions rpc/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import (
"strings"
"sync"
"unicode"
"unicode/utf8"

"github.com/ethereum/go-ethereum/log"
)
Expand Down Expand Up @@ -139,16 +138,14 @@ func newCallback(receiver, fn reflect.Value) *callback {
c := &callback{fn: fn, rcvr: receiver, errPos: -1, isSubscribe: isPubSub(fntype)}
// Determine parameter types. They must all be exported or builtin types.
c.makeArgTypes()
if !allExportedOrBuiltin(c.argTypes) {
return nil
}

// Verify return types. The function must return at most one error
// and/or one other non-error value.
outs := make([]reflect.Type, fntype.NumOut())
for i := 0; i < fntype.NumOut(); i++ {
outs[i] = fntype.Out(i)
}
if len(outs) > 2 || !allExportedOrBuiltin(outs) {
if len(outs) > 2 {
return nil
}
// If an error is returned, it must be the last returned value.
Expand Down Expand Up @@ -218,27 +215,6 @@ func (c *callback) call(ctx context.Context, method string, args []reflect.Value
return results[0].Interface(), nil
}

// Is this an exported - upper case - name?
func isExported(name string) bool {
rune, _ := utf8.DecodeRuneInString(name)
return unicode.IsUpper(rune)
}

// Are all those types exported or built-in?
func allExportedOrBuiltin(types []reflect.Type) bool {
for _, typ := range types {
for typ.Kind() == reflect.Ptr {
typ = typ.Elem()
}
// PkgPath will be non-empty even for an exported type,
// so we need to check the type name as well.
if !isExported(typ.Name()) && typ.PkgPath() != "" {
return false
}
}
return true
}

// Is t context.Context or *context.Context?
func isContextType(t reflect.Type) bool {
for t.Kind() == reflect.Ptr {
Expand Down
12 changes: 6 additions & 6 deletions rpc/subscription.go
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,11 @@ func (sub *ClientSubscription) Err() <-chan error {
// Unsubscribe unsubscribes the notification and closes the error channel.
// It can safely be called more than once.
func (sub *ClientSubscription) Unsubscribe() {
sub.quitWithError(nil, true)
sub.quitWithError(true, nil)
sub.errOnce.Do(func() { close(sub.err) })
}

func (sub *ClientSubscription) quitWithError(err error, unsubscribeServer bool) {
func (sub *ClientSubscription) quitWithError(unsubscribeServer bool, err error) {
sub.quitOnce.Do(func() {
// The dispatch loop won't be able to execute the unsubscribe call
// if it is blocked on deliver. Close sub.quit first because it
Expand Down Expand Up @@ -276,7 +276,7 @@ func (sub *ClientSubscription) start() {
sub.quitWithError(sub.forward())
}

func (sub *ClientSubscription) forward() (err error, unsubscribeServer bool) {
func (sub *ClientSubscription) forward() (unsubscribeServer bool, err error) {
cases := []reflect.SelectCase{
{Dir: reflect.SelectRecv, Chan: reflect.ValueOf(sub.quit)},
{Dir: reflect.SelectRecv, Chan: reflect.ValueOf(sub.in)},
Expand All @@ -298,14 +298,14 @@ func (sub *ClientSubscription) forward() (err error, unsubscribeServer bool) {

switch chosen {
case 0: // <-sub.quit
return nil, false
return false, nil
case 1: // <-sub.in
val, err := sub.unmarshal(recv.Interface().(json.RawMessage))
if err != nil {
return err, true
return true, err
}
if buffer.Len() == maxClientSubscriptionBuffer {
return ErrSubscriptionQueueOverflow, true
return true, ErrSubscriptionQueueOverflow
}
buffer.PushBack(val)
case 2: // sub.channel<-
Expand Down
15 changes: 8 additions & 7 deletions rpc/testservice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,24 +53,24 @@ func sequentialIDGenerator() func() ID {

type testService struct{}

type Args struct {
type echoArgs struct {
S string
}

type Result struct {
type echoResult struct {
String string
Int int
Args *Args
Args *echoArgs
}

func (s *testService) NoArgsRets() {}

func (s *testService) Echo(str string, i int, args *Args) Result {
return Result{str, i, args}
func (s *testService) Echo(str string, i int, args *echoArgs) echoResult {
return echoResult{str, i, args}
}

func (s *testService) EchoWithCtx(ctx context.Context, str string, i int, args *Args) Result {
return Result{str, i, args}
func (s *testService) EchoWithCtx(ctx context.Context, str string, i int, args *echoArgs) echoResult {
return echoResult{str, i, args}
}

func (s *testService) Sleep(ctx context.Context, duration time.Duration) {
Expand All @@ -81,6 +81,7 @@ func (s *testService) Rets() (string, error) {
return "", nil
}

//lint:ignore ST1008 returns error first on purpose.
func (s *testService) InvalidRets1() (error, string) {
return nil, ""
}
Expand Down
3 changes: 1 addition & 2 deletions rpc/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,8 @@ func (bn *BlockNumber) UnmarshalJSON(data []byte) error {
return err
}
if blckNum > math.MaxInt64 {
return fmt.Errorf("Blocknumber too high")
return fmt.Errorf("block number larger than int64")
}

*bn = BlockNumber(blckNum)
return nil
}
Expand Down
2 changes: 1 addition & 1 deletion rpc/websocket_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func TestWebsocketLargeCall(t *testing.T) {
defer client.Close()

// This call sends slightly less than the limit and should work.
var result Result
var result echoResult
arg := strings.Repeat("x", maxRequestContentLength-200)
if err := client.Call(&result, "test_echo", arg, 1); err != nil {
t.Fatalf("valid call didn't work: %v", err)
Expand Down