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

@@ -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.

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()

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)
+	})
 }