Repository: creachadair/jrpc2
Pull request #15 (2020-03-20 21:10:27)

server: add a one-shot service wrapper
Inspired by this comment thread: https://github.com/creachadair/jrpc2/pull/14#pullrequestreview-378053953

There are a couple differences worth noting here from the outline in that thread:

- Rather than exposing Wait and WaitStatus, this implementation runs the server to completion. A caller that doesn't want to wait can run it in a goroutine (`go s.Run(ch)`) and then the service alone is responsible for handling the return status.
- No work is done until the server is started, so that it's safe to abandon a constructed wrapper without orphaning resources the service may have allocated.

creachadair: There is (at least) one deficiency that I need to fix, however: A call to `Stop` races with `Run`. The way you're using that in your change that won't matter (I think) but I want to fix that before I merge it.

Maybe I should ask this on https://github.com/hashicorp/terraform-ls/commit/6ac1bbc16032a66d5d98879fb1675ef7d2136cbc, but I am curious why you are bothering with a context there at all? You don't have a timeout running, and the service doesn't take a context, so it seems like it would be simpler to just run the service synchronously. By the time `srv.Run()` returns you know it's complete.
radeksimko: > Maybe I should ask this on hashicorp/terraform-ls@6ac1bbc, but I am curious why you are bothering with a context there at all?

I'm not sure if we're talking about the same part of the code, but I think there's a few reasons for _service_ context in general:
 - cancellation from tests (either after each test, or to test that code being tested triggered the cancellation)
 - ability to stop the service without stopping the server from within a request
   - requests have their own context, but that's not quite the level of context I want to be cancelling
 - to allow long-running operations to get their own "service-lifetime" context which allows them to finish _after_ the response was dispatched (and request context cancelled), or be cancelled when the service is cancelled

In theory the service wouldn't need the parent context, that's currently only needed because that's how I pass global flag with log path all the way through:
https://github.com/hashicorp/terraform-ls/blob/362bd3e24054734428618d56c7cf90b20498f328/langserver/handlers/service.go#L91-L94
Maybe there is a better way, at which point the service could have its own isolated context - I'm not sure. The whole [service file](https://github.com/hashicorp/terraform-ls/blob/master/langserver/handlers/service.go) seems a little verbose to me and I may try refactoring it once I start adding more handlers.

If I misunderstood your question I apologise.
creachadair: > I'm not sure if we're talking about the same part of the code, but I think there's a few reasons for _service_ context in general

Apologies, I should have been more specific. I was looking at this code in particular: https://github.com/hashicorp/terraform-ls/commit/6ac1bbc#diff-57fb1c0a59f76a149c0b76eefa1d0bbeR63. Just above that at line 61, it derives a cancellable context from `ls.srvCtx`, then creates a fresh goroutine to run the server. But the server does not use this context, and cancelling the derived context does not affect the parent context—so there is not, as far as I can see, any need for that derived context. Relatedly, I don't see any need to call `Run` inside a separate goroutine, when the very next thing that happens is to block for the context to terminate.

If there were other events that could terminate the context (e.g., a ticker for a timeout, a deadline, etc.) that could make more sense. I mentioned it in this case only because it seems like maybe there's more code than necessary in that test. 🙂 


Patch:
diff --git a/server/simple.go b/server/simple.go
new file mode 100644
index 0000000..39e9f14
--- /dev/null
+++ b/server/simple.go
@@ -0,0 +1,43 @@
+package server
+
+import (
+	"errors"
+
+	"github.com/creachadair/jrpc2"
+	"github.com/creachadair/jrpc2/channel"
+)
+
+// A Simple server manages execution of a server for a single service instance.
+type Simple struct {
+	running bool
+	svc     Service
+	opts    *jrpc2.ServerOptions
+}
+
+// NewSimple constructs a new, unstarted *Simple instance for the given
+// service.  When run, the server will use the specified options.
+func NewSimple(svc Service, opts *jrpc2.ServerOptions) *Simple {
+	return &Simple{svc: svc, opts: opts}
+}
+
+// Run starts a server on the given channel, and blocks until it returns.  The
+// server exit status is reported to the service, and the error value returned.
+// Once Run returns, it can be run again with a new channel.
+//
+// If the caller does not need the error value and does not want to wait for
+// the server to complete, call Run in a goroutine.
+func (s *Simple) Run(ch channel.Channel) error {
+	if s.running { // sanity check
+		return errors.New("server is already running")
+	}
+	assigner, err := s.svc.Assigner()
+	if err != nil {
+		return err
+	}
+	s.running = true
+	srv := jrpc2.NewServer(assigner, s.opts).Start(ch)
+	stat := srv.WaitStatus()
+	s.svc.Finish(stat)
+	s.running = false
+	return stat.Err
+}


Test patch:
diff --git a/server/simple_test.go b/server/simple_test.go
new file mode 100644
index 0000000..378369e
--- /dev/null
+++ b/server/simple_test.go
@@ -0,0 +1,63 @@
+package server_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/creachadair/jrpc2"
+	"github.com/creachadair/jrpc2/channel"
+	"github.com/creachadair/jrpc2/handler"
+	"github.com/creachadair/jrpc2/server"
+)
+
+type testService struct {
+	assigner                   jrpc2.Assigner
+	assignCalled, finishCalled bool
+	stat                       jrpc2.ServerStatus
+}
+
+func (t *testService) Assigner() (jrpc2.Assigner, error) {
+	t.assignCalled = true
+	return t.assigner, nil
+}
+
+func (t *testService) Finish(stat jrpc2.ServerStatus) {
+	t.finishCalled = true
+	t.stat = stat
+}
+
+func TestSimple(t *testing.T) {
+	svc := &testService{assigner: handler.Map{
+		"Test": handler.New(func(ctx context.Context) string {
+			return "OK"
+		}),
+	}}
+	cpipe, spipe := channel.Direct()
+	cdone := make(chan struct{})
+	var result string
+	go func() {
+		defer close(cdone)
+		cli := jrpc2.NewClient(cpipe, nil)
+		defer cli.Close()
+		if err := cli.CallResult(context.Background(), "Test", nil, &result); err != nil {
+			t.Errorf("Call Test failed: %v", err)
+		}
+	}()
+
+	srv := server.NewSimple(svc, nil)
+	if err := srv.Run(spipe); err != nil {
+		t.Errorf("Server failed: %v", err)
+	}
+	if result != "OK" {
+		t.Errorf("Call result: got %q, want %q", result, "OK")
+	}
+	if !svc.assignCalled {
+		t.Error("Assigner was not called")
+	}
+	if !svc.finishCalled {
+		t.Error("Finish was not called")
+	}
+	if svc.stat.Err != nil {
+		t.Errorf("Server status: unexpected error: %+v", svc.stat)
+	}
+}