JSON RPC over HTTP #576

rossmcf · 2017-07-17T09:55:58Z

This PR continues @blainsmith's work from #451 to address issue #575.

This is still a work in progress, but I wanted to open the PR early, to invite discussion.

I've taken the liberty of staring a new fork. @blainsmith's commits have been imported, so that he's duly credited.

I've added some tests, and hopefully addressed some of the feedback from the previous PR, namely creating an RequestID object with a custom JSONUnmarshal, that can handle IDs as strings, ints or floats. @groob, I'd appreciate your thoughts on that.

I'll try to tackle some more of the feedback, regarding server and encoding options in due course.

Thanks in advance for any feedback you can offer.

Ross

Add basic server tests, following example from http transport. Switch Response.Error to pointer, to make absence clearer.

blainsmith · 2017-07-17T16:33:03Z

Thanks for the props and also moving it forward! I got pulled away to do other things and never got a chance to move forward with it.

peterbourgon · 2017-07-17T16:55:05Z

With the caveats that I don't know a whole lot about JSON RPC, and I've only just skimmed the code: the shape of this looks quite good to me. I'll have a few nits to pick in a final review, but otherwise 👍 from me so far.

basvanbeek · 2017-07-17T17:15:39Z

I'll try to take a look too. I have used JSON-RPC extensively in the past.

peterbourgon · 2017-07-26T03:33:03Z

@rossmcf Just ping me when you're ready for a final review! 👍

rossmcf · 2017-07-31T10:02:17Z

Thanks @peterbourgon. I should hopefully have some time to tidy it up towards the end of the week.

rossmcf · 2017-08-03T20:13:13Z

@peterbourgon I'm getting a little blind to this now, so would appreciate your input.

With regard to examples, would I be best to add a JSON RPC transport to the addsvc?

rossmcf · 2017-08-03T20:25:20Z

Also, any pointers on why the travis build is failing would be useful. I can't make sense of the output.

rjeczalik · 2017-08-03T21:21:12Z

I can't make sense of the output.

--- FAIL: TestDefaultEndpointer (0.03s)
	endpointer_test.go:33: want 2, have 0
	endpointer_test.go:39: endpoint a closed, good
	endpointer_test.go:45: endpoint b closed, good

This test is racy.

It expects the cache to have certain elements, however it's updated concurrently.

It's pretty easy to reproduce:

$ go get golang.org/x/tools/cmd/stress
$ go test -race -c .
$ stress ./sd.test 

/var/folders/dl/s7jq4vyn7358p1mtsmn3bk5w0000gn/T/go-stress749329897
--- FAIL: TestDefaultEndpointer (0.00s)
   endpointer_test.go:33: want 2, have 0
   endpointer_test.go:39: endpoint a closed, good
   endpointer_test.go:45: endpoint b closed, good
FAIL


ERROR: exit status 1

33 runs so far, 1 failures

fredipevcin · 2017-08-23T12:27:16Z

@rossmcf @peterbourgon What do you think about https://github.com/rossmcf/kit/compare/json-rpc-over-http...fredipevcin:json-rpc-over-http?expand=1

Idea is that method correlates to service.

For every service it is possible to specify logger, before and after instead of only for whole RPC handler.

handler := jsonrpc.NewServer(
    jsonrpc.ServiceMap{
        "add": jsonrpc.NewService(
            func(ctx context.Context, request interface{}) (response interface{}, err error) {
                return nil, nil
            },
            func(ctx context.Context, msg json.RawMessage) (interface{}, error) {
                return nil, nil
            },
            func(ctx context.Context, result interface{}) (json.RawMessage, error) {
                return []byte(`"result"`), nil
            },
            jsonrpc.ServiceBefore(func(ctx context.Context, _ http.Header) context.Context {
                return ctx
            }),
        ),
    },
)
http.Handle("/v1/rpc", handler)

Service is not aware about http.Request nor http.ResponseWritter because Server takes care of this.

Code is just WIP as I wanted your option on it if it makes sense.

fredipevcin · 2017-08-24T13:28:50Z

I'm currently working on JSON-RPC implementation https://github.com/fredipevcin/gokit-jsonrpc which we will use in-house. It is not yet completed.

peterbourgon · 2017-09-03T20:56:19Z

@rossmcf Thanks and please forgive my delay! At a glance this seems to be alright. Can you help me review this more thoroughly (and help future users!) by adding a runnable example? I think the best thing would be to add JSON RPC support to examples/addsvc, but I'd also look at something standalone if there were good reasons that weren't feasible.

rossmcf · 2018-02-02T17:21:50Z

Thanks for your feedback, everyone.

Test coverage in the package is looking more respectable now, and I think the issues raised have been addressed. Over to you.

peterbourgon

Two outstanding issues from my perspective. Once those are resolved I'm happy to merge!

peterbourgon · 2018-02-05T21:54:41Z

transport/http/jsonrpc/client.go

+	return &RequestID{
+		intValue: int(id),
+	}
+}


This isn't goroutine-safe, and the type that includes it (the client) doesn't perform any synchronization. You've got a data race! You need to either put synchronization in the client, and un-export this type or explicitly mark it as not goroutine-safe; or put synchronization in this type directly.

OK, I think that's addressed now, with an atomic.AddUint64 in Generate().

peterbourgon · 2018-02-05T21:56:03Z

transport/http/jsonrpc/server.go

+
+// ServerBefore functions are executed on the HTTP request object before the
+func ServerBefore(before ...httptransport.RequestFunc) ServerOption {
+	// request is decoded.


Still got a problem here.

peterbourgon · 2018-02-05T21:59:10Z

Oh, and the test failure reports

# github.com/go-kit/kit/transport/http/jsonrpc_test
transport/http/jsonrpc/client_test.go:119: Fatalf format %d has arg requestAtServer.ID of wrong type *github.com/go-kit/kit/transport/http/jsonrpc.RequestID
transport/http/jsonrpc/client_test.go:171: Fatalf format %d has arg rs of wrong type string

which I reckon is fixable, too.

Make auto-incrementing IDs goroutine safe. Make RequestIDGenerator interface public.

The client had been using the RequestID type in requests. Making this serialize in a deterministic and predictable way was going to be fiddly, so I decided to allow interface{} for IDs, client-side.

…over-http

rossmcf · 2018-02-16T16:52:53Z

OK, coveralls is finally satisfied. Hopefully that's everything taken care of.

peterbourgon · 2018-02-17T00:36:01Z

transport/http/jsonrpc/client_test.go

+		t.Fatalf("result is not string: (%T)%+v", result, result)
+	}
+	if rs != "boogaloo" {
+		t.Fatalf("want=boogaloo, got=%d", rs)


# github.com/go-kit/kit/transport/http/jsonrpc_test transport/http/jsonrpc/client_test.go:190: Fatalf format %d has arg rs of wrong type string FAIL github.com/go-kit/kit/transport/http/jsonrpc [build failed]

peterbourgon · 2018-02-17T00:36:26Z

Build failure due to an aggressive lint.

rossmcf · 2018-02-19T21:46:29Z

@peterbourgon Thanks for spotting that. Oddly, it didn't come up when I ran lint. I've made a few more tweaks to quieten (although not completely silence) the linter.

peterbourgon · 2018-02-19T23:50:23Z

Yes! Thank you so much! This is great. I'll cut a release this week, probably.

rossmcf · 2018-02-20T09:29:54Z

Thank you for all of your support with this.

In the release notes, might you be able to mention the original contribution of @blainsmith, and my employer, @senseyeio, on whose time I did most of the work for this?

sas1024 · 2018-03-05T09:18:42Z

Any plans for new release?

peterbourgon · 2018-03-06T03:27:25Z

Want to wait for 1 or 2 more inflight PRs to land. Sorry for the delay.

* first pass at JSON RPC HTTP transport * example implementation of JSON RPC over HTTP * Add ID type for JSON RPC Request, with tests. * Add basic server testing for JSON RPC. Add basic server tests, following example from http transport. Switch Response.Error to pointer, to make absence clearer. * Handle unregistered JSON RPC methods. * Package tidy-up. * Test ServerBefore / ServerAfter for JSON RPC. * More JSON RPC tests. * Remove JSON RPC from addsvc example, pending full JSON RPC example. * Remove JSON RPC from addsvc example, pending full JSON RPC example. * Remove context field from jsonrpc.Server. * Add JSON content type to all JSON RPC responses. * Add JSON content type to all JSON RPC responses. * Remove client-side JSON RPC funcs for now. * Document interceptingWriter * Add JSON RPC doc.go. * Add README for JSON RPC. * Wire in JSON RPC addsvc. * Add JSON RPC to Addsvc CLI. * Set JSONRPC version in responses. * Add JSON RPC client to addcli example. * Wire in client middlewares for JSON RPC addsvc example. * Fix rate limiter dependency. * Add concat JSON RPC method. * Improve JSON RPC server test coverage. * Add error tests. * Clarify ErrorCoder in comment. * Make endpoint consistent in README. * Gofmt handler example in README. * Auto-increment client IDs. Allow for customisation. * Add happy-path test for JSON RPC client. * Provide default encoder/decoder in JSON RPC client. * Fix comment line. * RequestIDGenerator tidy-up. Make auto-incrementing IDs goroutine safe. Make RequestIDGenerator interface public. * Fix client ID creation. The client had been using the RequestID type in requests. Making this serialize in a deterministic and predictable way was going to be fiddly, so I decided to allow interface{} for IDs, client-side. * Test client request ID more effectively. * Cover client options in test. * Improve error test coverage. * Fix format spec in test output. * Tweaks to satisfy the linter.

blainsmith and others added 11 commits July 9, 2017 20:01

first pass at JSON RPC HTTP transport

673600c

example implementation of JSON RPC over HTTP

53b2faa

Add ID type for JSON RPC Request, with tests.

7cfc4f1

Add basic server testing for JSON RPC.

ed2fbcf

Add basic server tests, following example from http transport. Switch Response.Error to pointer, to make absence clearer.

Handle unregistered JSON RPC methods.

4adaf39

Package tidy-up.

ea68d6a

Test ServerBefore / ServerAfter for JSON RPC.

111107e

More JSON RPC tests.

0090caa

Remove JSON RPC from addsvc example, pending full JSON RPC example.

40485ea

Remove JSON RPC from addsvc example, pending full JSON RPC example.

08a25d8

Remove context field from jsonrpc.Server.

0588817

rossmcf added 2 commits July 20, 2017 15:27

Add JSON content type to all JSON RPC responses.

5be17f5

Add JSON content type to all JSON RPC responses.

bcdbd54

rossmcf added 5 commits August 3, 2017 19:40

Remove client-side JSON RPC funcs for now.

526b570

Document interceptingWriter

773fed5

Add JSON RPC doc.go.

13719b2

Merge branch 'master' into json-rpc-over-http

1f0ab0b

Add README for JSON RPC.

dc16302

peterbourgon mentioned this pull request Aug 4, 2017

JSON RPC over HTTP Transport #451

Closed

rossmcf added 5 commits January 8, 2018 09:20

Make endpoint consistent in README.

bd64ffe

Gofmt handler example in README.

f391b57

Auto-increment client IDs. Allow for customisation.

01f4ca8

Add happy-path test for JSON RPC client.

c1abdf6

Provide default encoder/decoder in JSON RPC client.

b653275

vetcher mentioned this pull request Feb 5, 2018

Add jsonRPC transport devimteam/microgen#45

Open

peterbourgon requested changes Feb 5, 2018

View reviewed changes

rossmcf added 7 commits February 16, 2018 14:14

Fix comment line.

75f0455

RequestIDGenerator tidy-up.

6de79e1

Make auto-incrementing IDs goroutine safe. Make RequestIDGenerator interface public.

Fix client ID creation.

af7f7b5

The client had been using the RequestID type in requests. Making this serialize in a deterministic and predictable way was going to be fiddly, so I decided to allow interface{} for IDs, client-side.

Test client request ID more effectively.

033ad9c

Cover client options in test.

422cffa

Improve error test coverage.

7abfb23

Merge branch 'master' of https://github.com/go-kit/kit into json-rpc-…

131ff45

…over-http

peterbourgon reviewed Feb 17, 2018

View reviewed changes

rossmcf added 2 commits February 19, 2018 21:25

Fix format spec in test output.

1aecd5e

Tweaks to satisfy the linter.

124b430

peterbourgon approved these changes Feb 19, 2018

View reviewed changes

peterbourgon merged commit 3bfaad9 into go-kit:master Feb 19, 2018

peterbourgon mentioned this pull request Mar 7, 2018

JSON RPC Client doesn't handle RPC errors. #672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSON RPC over HTTP #576

JSON RPC over HTTP #576

rossmcf commented Jul 17, 2017

blainsmith commented Jul 17, 2017

peterbourgon commented Jul 17, 2017

basvanbeek commented Jul 17, 2017

peterbourgon commented Jul 26, 2017

rossmcf commented Jul 31, 2017

rossmcf commented Aug 3, 2017

rossmcf commented Aug 3, 2017

rjeczalik commented Aug 3, 2017

fredipevcin commented Aug 23, 2017

fredipevcin commented Aug 24, 2017

peterbourgon commented Sep 3, 2017

rossmcf commented Feb 2, 2018

peterbourgon left a comment

peterbourgon Feb 5, 2018

rossmcf Feb 16, 2018

peterbourgon Feb 5, 2018

peterbourgon commented Feb 5, 2018 •

edited

Loading

rossmcf commented Feb 16, 2018

peterbourgon Feb 17, 2018

peterbourgon commented Feb 17, 2018

rossmcf commented Feb 19, 2018

peterbourgon commented Feb 19, 2018

rossmcf commented Feb 20, 2018

sas1024 commented Mar 5, 2018

peterbourgon commented Mar 6, 2018

JSON RPC over HTTP #576

JSON RPC over HTTP #576

Conversation

rossmcf commented Jul 17, 2017

blainsmith commented Jul 17, 2017

peterbourgon commented Jul 17, 2017

basvanbeek commented Jul 17, 2017

peterbourgon commented Jul 26, 2017

rossmcf commented Jul 31, 2017

rossmcf commented Aug 3, 2017

rossmcf commented Aug 3, 2017

rjeczalik commented Aug 3, 2017

fredipevcin commented Aug 23, 2017

fredipevcin commented Aug 24, 2017

peterbourgon commented Sep 3, 2017

rossmcf commented Feb 2, 2018

peterbourgon left a comment

Choose a reason for hiding this comment

peterbourgon Feb 5, 2018

Choose a reason for hiding this comment

rossmcf Feb 16, 2018

Choose a reason for hiding this comment

peterbourgon Feb 5, 2018

Choose a reason for hiding this comment

peterbourgon commented Feb 5, 2018 • edited Loading

rossmcf commented Feb 16, 2018

peterbourgon Feb 17, 2018

Choose a reason for hiding this comment

peterbourgon commented Feb 17, 2018

rossmcf commented Feb 19, 2018

peterbourgon commented Feb 19, 2018

rossmcf commented Feb 20, 2018

sas1024 commented Mar 5, 2018

peterbourgon commented Mar 6, 2018

peterbourgon commented Feb 5, 2018 •

edited

Loading