From 7504fd22198d4a6fed994e56ae6795888bb880e4 Mon Sep 17 00:00:00 2001 From: Ian Cottrell Date: Tue, 21 Apr 2020 11:47:21 -0400 Subject: [PATCH] internal/lsp: fix broken lsp logs ID's are now by value not pointer, which caused it to not use the Format method, resulting in broken id strings The id maps need to be crossover (set and get go to different maps for a given direction of message) Change-Id: Ide2b63ec1b009ae3587ee10e8bce018732ea342c Reviewed-on: https://go-review.googlesource.com/c/tools/+/229244 Run-TryBot: Ian Cottrell TryBot-Result: Gobot Gobot Reviewed-by: Peter Weinberger --- gopls/integration/replay/main.go | 3 ++- internal/jsonrpc2/wire.go | 8 +++--- internal/jsonrpc2/wire_test.go | 20 +++------------ internal/lsp/protocol/log.go | 42 +++++++++++++------------------ internal/lsp/protocol/protocol.go | 4 +-- 5 files changed, 28 insertions(+), 49 deletions(-) diff --git a/gopls/integration/replay/main.go b/gopls/integration/replay/main.go index 49486ff72..cea66f2b2 100644 --- a/gopls/integration/replay/main.go +++ b/gopls/integration/replay/main.go @@ -145,7 +145,8 @@ func send(ctx context.Context, l *parse.Logmsg, stream jsonrpc2.Stream, id *json if err != nil { n = 0 } - id = jsonrpc2.NewIntID(int64(n)) + nid := jsonrpc2.NewIntID(int64(n)) + id = &nid } var msg jsonrpc2.Message var err error diff --git a/internal/jsonrpc2/wire.go b/internal/jsonrpc2/wire.go index 632c53194..cfef6c287 100644 --- a/internal/jsonrpc2/wire.go +++ b/internal/jsonrpc2/wire.go @@ -125,22 +125,20 @@ func (wireVersionTag) UnmarshalJSON(data []byte) error { const invalidID int64 = math.MaxInt64 // NewIntID returns a new numerical request ID. -func NewIntID(v int64) *ID { return &ID{number: v} } +func NewIntID(v int64) ID { return ID{number: v} } // NewStringID returns a new string request ID. -func NewStringID(v string) *ID { return &ID{name: v} } +func NewStringID(v string) ID { return ID{name: v} } // Format writes the ID to the formatter. // If the rune is q the representation is non ambiguous, // string forms are quoted, number forms are preceded by a # -func (id *ID) Format(f fmt.State, r rune) { +func (id ID) Format(f fmt.State, r rune) { numF, strF := `%d`, `%s` if r == 'q' { numF, strF = `#%d`, `%q` } switch { - case id == nil: - fmt.Fprintf(f, numF, invalidID) case id.name != "": fmt.Fprintf(f, strF, id.name) default: diff --git a/internal/jsonrpc2/wire_test.go b/internal/jsonrpc2/wire_test.go index 758b0bbae..7cddb6574 100644 --- a/internal/jsonrpc2/wire_test.go +++ b/internal/jsonrpc2/wire_test.go @@ -8,7 +8,6 @@ import ( "bytes" "encoding/json" "fmt" - "math" "testing" "golang.org/x/tools/internal/jsonrpc2" @@ -16,20 +15,13 @@ import ( var wireIDTestData = []struct { name string - id *jsonrpc2.ID + id jsonrpc2.ID encoded []byte plain string quoted string }{ { - name: `nil`, - id: nil, - encoded: []byte(`null`), - plain: fmt.Sprintf(`%d`, int64(math.MaxInt64)), - quoted: fmt.Sprintf(`#%d`, int64(math.MaxInt64)), - }, { name: `empty`, - id: &jsonrpc2.ID{}, encoded: []byte(`0`), plain: `0`, quoted: `#0`, @@ -64,7 +56,7 @@ func TestIDFormat(t *testing.T) { func TestIDEncode(t *testing.T) { for _, test := range wireIDTestData { t.Run(test.name, func(t *testing.T) { - data, err := json.Marshal(test.id) + data, err := json.Marshal(&test.id) if err != nil { t.Fatal(err) } @@ -81,12 +73,8 @@ func TestIDDecode(t *testing.T) { t.Fatal(err) } if got == nil { - if test.id != nil { - t.Errorf("got nil want %s", test.id) - } - } else if test.id == nil { - t.Errorf("got %s want nil", got) - } else if *got != *test.id { + t.Errorf("got nil want %s", test.id) + } else if *got != test.id { t.Errorf("got %s want %s", got, test.id) } }) diff --git a/internal/lsp/protocol/log.go b/internal/lsp/protocol/log.go index 1f9713463..328336f36 100644 --- a/internal/lsp/protocol/log.go +++ b/internal/lsp/protocol/log.go @@ -27,7 +27,7 @@ func (s *loggingStream) Read(ctx context.Context) (jsonrpc2.Message, int64, erro if err == nil { s.logMu.Lock() defer s.logMu.Unlock() - logIn(s.log, msg) + logCommon(s.log, msg, true) } return msg, count, err } @@ -35,7 +35,7 @@ func (s *loggingStream) Read(ctx context.Context) (jsonrpc2.Message, int64, erro func (s *loggingStream) Write(ctx context.Context, msg jsonrpc2.Message) (int64, error) { s.logMu.Lock() defer s.logMu.Unlock() - logOut(s.log, msg) + logCommon(s.log, msg, false) count, err := s.stream.Write(ctx, msg) return count, err } @@ -60,23 +60,19 @@ var maps = &mapped{ // these 4 methods are each used exactly once, but it seemed // better to have the encapsulation rather than ad hoc mutex // code in 4 places -func (m *mapped) client(id string, del bool) req { +func (m *mapped) client(id string) req { m.mu.Lock() defer m.mu.Unlock() v := m.clientCalls[id] - if del { - delete(m.clientCalls, id) - } + delete(m.clientCalls, id) return v } -func (m *mapped) server(id string, del bool) req { +func (m *mapped) server(id string) req { m.mu.Lock() defer m.mu.Unlock() v := m.serverCalls[id] - if del { - delete(m.serverCalls, id) - } + delete(m.serverCalls, id) return v } @@ -94,7 +90,13 @@ func (m *mapped) setServer(id string, r req) { const eor = "\r\n\r\n\r\n" -func logCommon(outfd io.Writer, msg jsonrpc2.Message, direction, pastTense string) { +func logCommon(outfd io.Writer, msg jsonrpc2.Message, isRead bool) { + direction, pastTense := "Received", "Received" + get, set := maps.client, maps.setServer + if isRead { + direction, pastTense = "Sending", "Sent" + get, set = maps.server, maps.setClient + } if msg == nil || outfd == nil { return } @@ -108,7 +110,7 @@ func logCommon(outfd io.Writer, msg jsonrpc2.Message, direction, pastTense strin id := fmt.Sprint(msg.ID()) fmt.Fprintf(&buf, "%s request '%s - (%s)'.\n", direction, msg.Method(), id) fmt.Fprintf(&buf, "Params: %s%s", msg.Params(), eor) - maps.setServer(id, req{method: msg.Method(), start: tm}) + set(id, req{method: msg.Method(), start: tm}) case *jsonrpc2.Notification: fmt.Fprintf(&buf, "%s notification '%s'.\n", direction, msg.Method()) fmt.Fprintf(&buf, "Params: %s%s", msg.Params(), eor) @@ -118,21 +120,11 @@ func logCommon(outfd io.Writer, msg jsonrpc2.Message, direction, pastTense strin fmt.Fprintf(outfd, "[Error - %s] %s #%s %s%s", pastTense, tmfmt, id, err, eor) return } - cc := maps.client(id, true) + cc := get(id) elapsed := tm.Sub(cc.start) - fmt.Fprintf(&buf, "Received response '%s - (%s)' in %dms.\n", - cc.method, id, elapsed/time.Millisecond) + fmt.Fprintf(&buf, "%s response '%s - (%s)' in %dms.\n", + direction, cc.method, id, elapsed/time.Millisecond) fmt.Fprintf(&buf, "Result: %s%s", msg.Result(), eor) } outfd.Write([]byte(buf.String())) } - -// Writing a message to the client, log it -func logOut(outfd io.Writer, msg jsonrpc2.Message) { - logCommon(outfd, msg, "Received", "Received") -} - -// Got a message from the client, log it -func logIn(outfd io.Writer, msg jsonrpc2.Message) { - logCommon(outfd, msg, "Sending", "Sent") -} diff --git a/internal/lsp/protocol/protocol.go b/internal/lsp/protocol/protocol.go index 92e3d3319..8dded0526 100644 --- a/internal/lsp/protocol/protocol.go +++ b/internal/lsp/protocol/protocol.go @@ -49,9 +49,9 @@ func CancelHandler(handler jsonrpc2.Handler) jsonrpc2.Handler { return sendParseError(ctx, reply, err) } if n, ok := params.ID.(float64); ok { - canceller(*jsonrpc2.NewIntID(int64(n))) + canceller(jsonrpc2.NewIntID(int64(n))) } else if s, ok := params.ID.(string); ok { - canceller(*jsonrpc2.NewStringID(s)) + canceller(jsonrpc2.NewStringID(s)) } else { return sendParseError(ctx, reply, fmt.Errorf("request ID %v malformed", params.ID)) }