More server API updates #88

johningve · 2020-07-07T14:45:37Z

This PR changes the server API again...

Instead of using channels to send responses, we can use a simple func that embeds the message in metadata and forwards onto the correct channel. Thus, we no longer have to create a new goroutine for every single message.

This PR also adds the server stream context as a parameter to each handler, as well as new manager options for setting metadata. The metadata can be extracted from the stream context:

metadata.FromIncomingContext(ctx)

In addition, I have found a way to make the TestSetup helper useful for GorumsServer types.

The new server API:

func (srv *orderedServer) OrderedQC(_ context.Context, in *Echo, out func(*Echo)) {
	out(in)
}

~~Am leaving this as a draft for now, as one of the tests is failing. Will investigate tomorrow...~~

EDIT: failing ordering test was due to indexing QF map incorrectly.

Instead of sending replies from server handlers on a channel that is then read by a separate goroutine, we can use a function to process the reply and send it. This way we avoid creating unnecessary goroutines.

Also exposes the context of NodeStream in each handler. This allows for some metadata to be accessed on the server side. The metadata is only set once, when the client connects to the server.

TestSetup can now work with any server type that implements the Serve() and Stop() functions.

By giving the port number '0', we can get an available port selected by the system.

Since we're now using maps in QF, we can no longer simply return index 0

The server-side metadata allows us to check the auth type of the client. This commit adds a test to see if TLS is working with Gorums. Also, turns out that the reason the old TLS test broke was because the included cert had expired. To avoid this problem in the future, the test now generates its own certificate.

johningve · 2020-07-09T08:40:02Z

@meling I added a new test for TLS. I think that the old test broke because the included certificate expired. To avoid that problem, the new test generates its own TLS certificate.

Changed some values to make it similar to the certs generated by https://golang.org/src/crypto/tls/generate_cert.go

johningve · 2020-07-09T15:12:33Z

@meling I've also added a test that verifies that reconnection of the streams is working. Let me know if you want me to break some of these changes out into separate pull requests.

johningve · 2020-07-09T15:20:27Z

Also, should we consider not including all of the generated code for the tests, and rely on the Makefile instead? The amount of code that gets updated from a single change in gorums is quite large now. We could set up a Makefile target to run all of the tests and then set up a GitHub action to run the tests automatically.

meling · 2020-07-09T18:35:23Z

testing_gorums.go

 // Package testing provide a public API for setting up Gorums.
 // This package can be used by other packages, such as Raft and HotStuff.

 // TestSetup starts numServers gRPC servers using the given registration
 // function, and returns the server addresses along with a stop function
 // that should be called to shut down the test.
-func TestSetup(t testing.TB, numServers int, regSrvFn func(*grpc.Server)) ([]string, func()) {
+func TestSetup(t testing.TB, numServers int, srvFunc func() interface{}) ([]string, func()) {


Since the TestSetup function only works srvFunc() that match the server interface, we could export the interface as gorums.Server. However, this creates an exported interface that is only used for this one test method. I think I would be happier if this TestSetup method and the Server interface were placed in a separate package, different from the gorums root package. I originally placed it in a package testing, but that caused naming problems with the std lib testing package. Right now, I'm not able to come up with a good name. Maybe it would be solved if we renamed the imported gorums/testing package to gt or something.
@Raytar What's your take?

On the other hand, I actually like calling gorums.TestSetup.

meling · 2020-07-09T19:17:37Z

tests/metadata/metadata_test.go

+	md, ok := metadata.FromIncomingContext(ctx)
+	if !ok {
+		srv.t.Error("Failed to obtain metadata!")
+		out(&NodeID{})


This pattern has me a bit worried. Normally, we would return an error to the client here, e.g.:

return status.Errorf(codes.InvalidArgument, "Failed to obtain...")

Not sure how we should handle this. I was thinking that the error could be passed to the out func, but this won't work.

meling · 2020-07-09T19:26:02Z

tests/metadata/metadata_test.go

+		}
+
+		if resp.GetID() != node.ID() {
+			t.Fatalf("Wrong message")


Should follow the test pattern linked to from issue #90

meling · 2020-07-09T19:26:39Z

tests/metadata/metadata_test.go

+		t.Fatalf("Failed to execute RPC: %v", err)
+	}
+
+	if resp.GetID() != 1 {


Should follow test pattern in issue #90

meling · 2020-07-09T21:11:58Z

cmd/protoc-gen-gorums/gengorums/template_server.go

-			finished <- &gorumsMessage{metadata: in.metadata, message: resp}
-		}()
-		srv.{{.GoName}}(req, c)
+		once := new({{use "sync.Once" $genFile}})


Could you add a brief comment here explaining why we use the once.Do() thing?

Also, does the finished channel mean the queue of messages to be sent on the stream? I think here it sounds okay to use finished (maybe), but in the NodeStream implementation, it is also called finished. Maybe this should be called something else? (outQueue and sendQueue?)

meling · 2020-07-09T21:20:01Z

Also, should we consider not including all of the generated code for the tests, and rely on the Makefile instead? The amount of code that gets updated from a single change in gorums is quite large now. We could set up a Makefile target to run all of the tests and then set up a GitHub action to run the tests automatically.

@Raytar Agree with this. I've been wanting to set up CI testing, as mentioned in #90. We should still keep the zorums* generated files in dev, since they are required to compile the static files.

Edit: I've started (a bit late, so won't finish now) to rewrite the stability checker in the Makefile. I'm invoking the protoc func from a TestGorumsStability (see below). I plan to do the same for the other gentest targets. That way, we don't need the tricky Makefile targets...

func TestGorumsStability(t *testing.T) {
	t.Log("Running protoc test with source files expected to remain stable (no output change between runs)")
	// Go 1.15 specific funcs:
	dir1 := t.TempDir()
	dir2 := t.TempDir()
	// cp zorums.proto dir1
	// cp zorums.proto dir2
	protoc("dir1/zorums.proto")
	protoc("dir2/zorums.proto")
	// diff dir1 dir2
	// return error if different; maybe use cmp package to show differences.
}

meling · 2020-07-09T21:26:31Z

Thanks for adding tests and the TLS cert generation. Maybe it will be easier to keep multiple PRs, if we don't check in all the generated code. I guess we will still get conflicts in the zorums* files.

johningve · 2020-07-10T11:50:48Z

Closing in favor of #91 and #92.

johningve added 10 commits July 7, 2020 13:39

server handler: Use a func instead of a channel

edee8ab

Instead of sending replies from server handlers on a channel that is then read by a separate goroutine, we can use a function to process the reply and send it. This way we avoid creating unnecessary goroutines.

Add options to include metadata in NodeStream

406e30c

Also exposes the context of NodeStream in each handler. This allows for some metadata to be accessed on the server side. The metadata is only set once, when the client connects to the server.

Make TestSetup work for generic server types

7b3f009

TestSetup can now work with any server type that implements the Serve() and Stop() functions.

Added tests for server metadata

e74d456

tests/ordering: use TestSetup()

105ff7a

tests/qf: use new TestSetup()

ea63869

tests/ordering: use TestSetup()

701fe18

Update benchmark

fd597e2

TestSetup: use automatic port number

cf73db2

By giving the port number '0', we can get an available port selected by the system.

tests/ordering: removed dead code and fixed QF

5794e13

Since we're now using maps in QF, we can no longer simply return index 0

johningve force-pushed the server-metadata branch from 14288a9 to 5794e13 Compare July 8, 2020 06:53

johningve added 4 commits July 8, 2020 09:08

Update userguide

30ff05c

update ordering.md

de3b4f7

Update example

47b8c4f

updated userguide client code

27e3b0f

johningve marked this pull request as ready for review July 8, 2020 07:48

johningve requested a review from meling July 8, 2020 07:48

johningve added 2 commits July 8, 2020 10:01

tests/metadata: test that getting peer info works

aa99a65

johningve added 2 commits July 9, 2020 12:10

tests/tls: made some adjustments to generated cert

cfd1902

Changed some values to make it similar to the certs generated by https://golang.org/src/crypto/tls/generate_cert.go

Added a test for reconnection

69ee3e0

meling requested changes Jul 9, 2020

View reviewed changes

johningve closed this Jul 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More server API updates #88

More server API updates #88

johningve commented Jul 7, 2020 •

edited

Loading

johningve commented Jul 9, 2020

johningve commented Jul 9, 2020

johningve commented Jul 9, 2020

meling Jul 9, 2020

meling Jul 9, 2020

meling Jul 9, 2020

meling Jul 9, 2020

meling Jul 9, 2020

meling Jul 9, 2020

meling commented Jul 9, 2020 •

edited

Loading

meling commented Jul 9, 2020

johningve commented Jul 10, 2020

More server API updates #88

More server API updates #88

Conversation

johningve commented Jul 7, 2020 • edited Loading

johningve commented Jul 9, 2020

johningve commented Jul 9, 2020

johningve commented Jul 9, 2020

meling Jul 9, 2020

Choose a reason for hiding this comment

meling Jul 9, 2020

Choose a reason for hiding this comment

meling Jul 9, 2020

Choose a reason for hiding this comment

meling Jul 9, 2020

Choose a reason for hiding this comment

meling Jul 9, 2020

Choose a reason for hiding this comment

meling Jul 9, 2020

Choose a reason for hiding this comment

meling commented Jul 9, 2020 • edited Loading

meling commented Jul 9, 2020

johningve commented Jul 10, 2020

johningve commented Jul 7, 2020 •

edited

Loading

meling commented Jul 9, 2020 •

edited

Loading