server/prpc: updated server according to protocol

server/prpc/encoding.go

Index: server/prpc/encoding.go
diff --git a/server/prpc/encoding.go b/server/prpc/encoding.go
index 2788e2e29d1d8b4a2a4b50380368de5e54399482..6ce914c6fc3281df3377e7879ad6e8897e0e6f60 100644
--- a/server/prpc/encoding.go
+++ b/server/prpc/encoding.go
@@ -8,7 +8,6 @@ package prpc
 
 import (
 	"bytes"
-	"io"
 	"net/http"
 	"sort"
 
@@ -17,18 +16,17 @@ import (
 	"golang.org/x/net/context"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
-
-	"github.com/luci/luci-go/common/logging"
 )
 
 const (
 	headerAccept = "Accept"
+	csrfPrefix   = ")]}'"

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

I believe this should end in a newline, both for clarity and Gerrit parity.

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

Done.

 )
 
 // responseFormat returns the format to be used in a response.
 // Can return only formatBinary (preferred), formatJSONPB or formatText.
-// In case of an error, format is undefined and the error has an HTTP status.
-func responseFormat(acceptHeader string) (format, *httpError) {
+// In case of an error, format is undefined.
+func responseFormat(acceptHeader string) (format, *protocolError) {
 	if acceptHeader == "" {
 		return formatBinary, nil
 	}
@@ -77,41 +75,63 @@ func responseFormat(acceptHeader string) (format, *httpError) {
 	return formats[0].Format, nil
 }
 
-// writeMessage writes a protobuf message to response in the specified format.
-func writeMessage(w http.ResponseWriter, msg proto.Message, format format) error {
+// respondMessage encodes msg to a response in the specified format.
+func respondMessage(msg proto.Message, format format) *response {
 	if msg == nil {
-		panic("msg is nil")
+		panicf("msg is nil")

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

nit: panic would work here, no need for "panicf".

Although since we return pRPC errors for encoding failures, maybe this should be
a status.Internal instead of a panic?

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

err, in another CL you said that we panic with errors, not strings. This the
sole reason I have panicf. Removed it.

This can only happen if caller did not check what is passes (it does), but
whatever, returned err response

[dnj, 2016/01/26 18:50:27]

Ah sorry I meant something like: panic(errors.New("...")), still return errors.
Maybe make panicf => panicErr(string)? Don't really care that much, so this is a
nit.
Thanks:)

 	}
 	var (
-		contentType string
-		res         []byte
-		err         error
+		res response
+		err error
 	)
 	switch format {
 	case formatBinary:
-		contentType = mtPRPCBinary
-		res, err = proto.Marshal(msg)
+		res.contentType = mtPRPCBinary
+		res.body, err = proto.Marshal(msg)
 
 	case formatJSONPB:
-		contentType = mtPRPCJSNOPB
-		m := jsonpb.Marshaler{Indent: "\t"}
+		res.contentType = mtPRPCJSNOPB
 		var buf bytes.Buffer
+		buf.WriteString(csrfPrefix)
+		m := jsonpb.Marshaler{Indent: "\t"}

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

1) Don't we normally use spaces to indent JSON?
2) Why format the JSON at all here? It's extra data to send. If we want
formatted JSON, there are tools to do that on the client side.

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

1) \t is less bytes
2) I used to curl in the past. I don't mind formatting it in rpc. Done

 		err = m.Marshal(&buf, msg)
-		buf.WriteString("\n")
-		res = buf.Bytes()
+		res.body = buf.Bytes()
+		res.newLine = true
 
 	case formatText:
-		contentType = mtPRPCText
+		res.contentType = mtPRPCText
 		var buf bytes.Buffer
 		err = proto.MarshalText(&buf, msg)
-		res = buf.Bytes()
+		res.body = buf.Bytes()
 	}
 	if err != nil {
-		return err
+		return errResponse(codes.Internal, 0, err.Error())
+	}
+
+	return &res
+}
+
+// respondProtocolError creates a response for a pRPC protocol error.
+func respondProtocolError(err *protocolError) *response {
+	return errResponse(codes.InvalidArgument, err.status, err.err.Error())
+}
+
+// errorCode returns a most appropriate gRPC code for an error
+func errorCode(err error) codes.Code {
+	switch err {
+	case nil:
+		panicf("err is nil")

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

Default to something like Unknown if nil, rather than panic.

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

returned ok

+		fallthrough
+
+	case context.DeadlineExceeded:
+		return codes.DeadlineExceeded
+
+	case context.Canceled:
+		return codes.Canceled
+
+	default:
+		return grpc.Code(err)
 	}
-	w.Header().Set(headerContentType, contentType)
-	_, err = w.Write(res)
-	return err
 }
 
 // codeToStatus maps gRPC codes to HTTP statuses.
@@ -138,70 +158,11 @@ var codeToStatus = map[codes.Code]int{
 	codes.DataLoss:           http.StatusInternalServerError,
 }
 
-// ErrorStatus returns HTTP status for an error.
-// In particular, it maps gRPC codes to HTTP statuses.
-// Status of nil is 200.
-//
-// See also grpc.Code.
-func ErrorStatus(err error) int {
-	if err, ok := err.(*httpError); ok {
-		return err.status
-	}
-
-	status, ok := codeToStatus[grpc.Code(err)]
-	if !ok {
-		status = http.StatusInternalServerError
-	}
-	return status
-}
-
-// ErrorDesc returns the error description of err if it was produced by pRPC or gRPC.
-// Otherwise, it returns err.Error() or empty string when err is nil.
-//
-// See also grpc.ErrorDesc.
-func ErrorDesc(err error) string {
-	if err == nil {
-		return ""
-	}
-	if e, ok := err.(*httpError); ok {
-		err = e.err
-	}
-	return grpc.ErrorDesc(err)
-}
-
-// writeError writes an error to an HTTP response.
-//
-// HTTP status is determined by ErrorStatus.
-// If it is http.StatusInternalServerError, prints only "Internal server error",
-// otherwise uses ErrorDesc.
-//
-// Logs all errors with status >= 500.
-func writeError(c context.Context, w http.ResponseWriter, err error) {
-	if err == nil {
-		panic("err is nil")
-	}
-
-	status := ErrorStatus(err)
-	if status >= 500 {
-		logging.Errorf(c, "HTTP %d: %s", status, ErrorDesc(err))
-	}
-
-	w.Header().Set(headerContentType, "text/plain")
-	w.WriteHeader(status)
-
-	var body string
-	if status == http.StatusInternalServerError {
-		body = "Internal server error"
-	} else {
-		body = ErrorDesc(err)
-	}
-	if _, err := io.WriteString(w, body+"\n"); err != nil {
-		logging.Errorf(c, "could not write error: %s", err)
-	}
-}
-
-func assert(condition bool) {
-	if !condition {
-		panicf("assertion failed")
+// codeStatus maps gRPC codes to HTTP status codes.
+// Falls back to http.StatusInternalServerError.
+func codeStatus(code codes.Code) int {

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

If it falls back to internal server error, you should remove the entries in the
map that map to internal server error.

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

Done.

+	if status, ok := codeToStatus[code]; ok {
+		return status
 	}
+	return http.StatusInternalServerError
 }