From e1febcf6093bfab21830a1f18bc55e8787b7f538 Mon Sep 17 00:00:00 2001
From: Suzy Mueller <suzmue@golang.org>
Date: Thu, 8 Jul 2021 01:27:54 -0400
Subject: [PATCH] service/dap: send dap error response on dap error (#2561)

The code previously expected the server to close when receiving
a message that it could not decode. However, there may be changes to
the spec that make new requests that we have not handled yet. In
that case, it would be preferable to return an error and continue handling
incoming requests.
---
 service/dap/daptest/client.go |  9 +++++++++
 service/dap/server.go         | 14 ++++++++++----
 service/dap/server_test.go    | 15 ++++++++++++++-
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/service/dap/daptest/client.go b/service/dap/daptest/client.go
index 3ad61ae1..53ba1b25 100644
--- a/service/dap/daptest/client.go
+++ b/service/dap/daptest/client.go
@@ -511,6 +511,15 @@ func (c *Client) UnknownEvent() {
 	c.send(event)
 }
 
+// BadRequest triggers an unmarshal error.
+func (c *Client) BadRequest() {
+	content := []byte("{malformedString}")
+	contentLengthHeaderFmt := "Content-Length: %d\r\n\r\n"
+	header := fmt.Sprintf(contentLengthHeaderFmt, len(content))
+	c.conn.Write([]byte(header))
+	c.conn.Write(content)
+}
+
 // KnownEvent passes decode checks, but delve has no 'case' to
 // handle it. This behaves the same way a new request type
 // added to go-dap, but not to delve.
diff --git a/service/dap/server.go b/service/dap/server.go
index d88b832c..f9fe0fc4 100644
--- a/service/dap/server.go
+++ b/service/dap/server.go
@@ -358,18 +358,24 @@ func (s *Server) serveDAPCodec() {
 	s.reader = bufio.NewReader(s.conn)
 	for {
 		request, err := dap.ReadProtocolMessage(s.reader)
-		// TODO(polina): Differentiate between errors and handle them
-		// gracefully. For example,
+		// Handle dap.DecodeProtocolMessageFieldError errors gracefully by responding with an ErrorResponse.
+		// For example:
 		// -- "Request command 'foo' is not supported" means we
 		// potentially got some new DAP request that we do not yet have
 		// decoding support for, so we can respond with an ErrorResponse.
-		// TODO(polina): to support this add Seq to
-		// dap.DecodeProtocolMessageFieldError.
+		//
+		// Other errors, such as unmarshalling errors, will log the error and cause the server to trigger
+		// a stop.
 		if err != nil {
 			select {
 			case <-s.stopTriggered:
 			default:
 				if err != io.EOF {
+					if decodeErr, ok := err.(*dap.DecodeProtocolMessageFieldError); ok {
+						// Send an error response to the users if we were unable to process the message.
+						s.sendInternalErrorResponse(decodeErr.Seq, err.Error())
+						continue
+					}
 					s.log.Error("DAP error: ", err)
 				}
 				s.triggerServerStop()
diff --git a/service/dap/server_test.go b/service/dap/server_test.go
index e7a0c31f..615a8f1a 100644
--- a/service/dap/server_test.go
+++ b/service/dap/server_test.go
@@ -4869,7 +4869,7 @@ func TestBadlyFormattedMessageToServer(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		// Send a badly formatted message to the server, and expect it to close the
 		// connection.
-		client.UnknownRequest()
+		client.BadRequest()
 		time.Sleep(100 * time.Millisecond)
 
 		_, err := client.ReadMessage()
@@ -4878,4 +4878,17 @@ func TestBadlyFormattedMessageToServer(t *testing.T) {
 			t.Errorf("got err=%v, want io.EOF", err)
 		}
 	})
+	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
+		// Send an unknown request message to the server, and expect it to send
+		// an error response.
+		client.UnknownRequest()
+		err := client.ExpectErrorResponse(t)
+		if err.Body.Error.Format != "Internal Error: Request command 'unknown' is not supported (seq: 1)" || err.RequestSeq != 1 {
+			t.Errorf("got %v, want  RequestSeq=1 Error=\"Internal Error: Request command 'unknown' is not supported (seq: 1)\"", err)
+		}
+
+		// Make sure that the unknown request did not kill the server.
+		client.InitializeRequest()
+		client.ExpectInitializeResponse(t)
+	})
 }
-- 
GitLab