server/prpc: updated server according to protocol

server/prpc/response.go

Index: server/prpc/response.go
diff --git a/server/prpc/response.go b/server/prpc/response.go
new file mode 100644
index 0000000000000000000000000000000000000000..abe1989aa5ba612bc9b46420e3a682bd915143d2
--- /dev/null
+++ b/server/prpc/response.go
@@ -0,0 +1,80 @@
+// Copyright 2016 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package prpc
+
+import (
+	"net/http"
+	"strconv"
+
+	"golang.org/x/net/context"
+	"google.golang.org/grpc/codes"
+
+	"github.com/luci/luci-go/common/logging"
+	"github.com/luci/luci-go/common/prpc"
+)
+
+var newLineBytes = []byte{'\n'}
+
+// response is a pRPC server response.
+// All pRPC responses must be written using write.
+type response struct {
+	code        codes.Code // defaults to OK
+	status      int        // defaults to status derived from code.
+	contentType string     // defaults to text/plain

[dnj (Google), 2016/01/26 16:13:56]

Shouldn't default. Have each method set this explicitly.

Either let the method specify the headers directly (including content-type) by
having a Headers element here, or condition:
if contentType != "" {
  //set it
}
// move on

[nodir, 2016/01/26 18:01:07]

Done.

+	body        []byte     // must be non-nil, otherwise write will panic.

[dnj (Google), 2016/01/26 16:13:56]

Is an empty protobuf not acceptable? I see no need to enforce that nil => panic.

[nodir, 2016/01/26 18:01:07]

empty and nil are two different things, but ok because zero values are cool

+	newLine     bool       // whether to write \n after body
+}
+
+// errResponse creates a response with an error.
+func errResponse(code codes.Code, status int, msg string) *response {
+	return &response{
+		code:    code,
+		status:  status,
+		body:    []byte(msg),
+		newLine: true,
+	}
+}
+
+// write writes r to w.
+func (r *response) write(c context.Context, w http.ResponseWriter) {
+	if r.body == nil {
+		panicf("res.body is nil")
+	}
+
+	if r.code == codes.Internal || r.code == codes.Unknown {

[dnj (Google), 2016/01/26 16:13:56]

nit: Use switch here.

[nodir, 2016/01/26 18:01:07]

so I counted lines and switch is 1 line more, but whatever

+		// res.body is error message.
+		logging.Errorf(c, "%s error: %s", r.code, r.body)

[dnj (Google), 2016/01/26 16:13:57]

nit: include fields

[nodir, 2016/01/26 18:01:07]

Done, also included http status

[nodir, 2016/01/26 18:08:13]

actually did not include status

+	}
+
+	w.Header().Set(prpc.HeaderGRPCCode, strconv.Itoa(int(r.code)))
+
+	if r.contentType == "" {
+		w.Header().Set(headerContentType, "text/plain")
+	} else {
+		w.Header().Set(headerContentType, r.contentType)
+	}
+
+	if r.status == 0 {
+		w.WriteHeader(codeStatus(r.code))
+	} else {
+		w.WriteHeader(r.status)
+	}
+
+	var body []byte

[dnj (Google), 2016/01/26 16:13:56]

nit: merge this with the block above, no need to switch on the same values
twice.

[nodir, 2016/01/26 18:01:07]

Done.

+	if r.code == codes.Internal || r.code == codes.Unknown {
+		body = []byte("Internal Server Error")
+	} else {
+		body = r.body
+	}
+
+	if _, err := w.Write(body); err != nil {
+		logging.WithError(err).Errorf(c, "Could not respond")
+		// The header is already written. There is nothing more we can do.
+		return
+	}
+	if r.newLine {
+		w.Write(newLineBytes)
+	}
+}