fix the http protocol dumper (#705)

we were using the httputil.DumpRequest when there is a perfectly good
req.Write method hanging out in the stdlib, that even does the chunked thing
that a few people ran into if they don't provide a content length:
https://golang.org/pkg/net/http/#Request.Write -- so we shouldn't run into
that issue again. I hit this in testing and it was not very fun to debug, so
added a test that repro'd it on master and fixes it here. of course, adding a
content length works too. tested this and it appears to work pretty well, also
cleaned up the control flow a little bit in http protocol.

api/agent/agent_test.go

@@ -1,7 +1,9 @@
 package agent
 
 import (
+	"bufio"
 	"bytes"
+	"encoding/json"
 	"fmt"
 	"io"
 	"net/http"
@@ -407,3 +409,89 @@ func TestSubmitError(t *testing.T) {
 		t.Fatal("expected error string to be set on call")
 	}
 }
+
+// this implements io.Reader, but importantly, is not a strings.Reader or
+// a type of reader than NewRequest can identify to set the content length
+// (important, for some tests)
+type dummyReader struct {
+	inner io.Reader
+}
+
+func (d *dummyReader) Read(b []byte) (int, error) {
+	return d.inner.Read(b)
+}
+
+func TestHTTPWithoutContentLengthWorks(t *testing.T) {
+	// TODO it may be a good idea to mock out the http server and use a real
+	// response writer with sync, and also test that this works with async + log
+
+	appName := "myapp"
+	path := "/hello"
+	url := "http://127.0.0.1:8080/r/" + appName + path
+
+	// we need to load in app & route so that FromRequest works
+	ds := datastore.NewMockInit(
+		[]*models.App{
+			{Name: appName},
+		},
+		[]*models.Route{
+			{
+				Path:        path,
+				AppName:     appName,
+				Image:       "fnproject/fn-test-utils",
+				Type:        "sync",
+				Format:      "http", // this _is_ the test
+				Timeout:     5,
+				IdleTimeout: 10,
+				Memory:      128,
+			},
+		}, nil,
+	)
+
+	a := New(NewDirectDataAccess(ds, ds, new(mqs.Mock)))
+	defer a.Close()
+
+	bodOne := `{"echoContent":"yodawg"}`
+
+	// get a req that uses the dummy reader, so that this can't determine
+	// the size of the body and set content length (user requests may also
+	// forget to do this, and we _should_ read it as chunked without issue).
+	req, err := http.NewRequest("GET", url, &dummyReader{strings.NewReader(bodOne)})
+	if err != nil {
+		t.Fatal("unexpected error building request", err)
+	}
+
+	// grab a buffer so we can read what gets written to this guy
+	var out bytes.Buffer
+	callI, err := a.GetCall(FromRequest(appName, path, req), WithWriter(&out))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	err = a.Submit(callI)
+	if err != nil {
+		t.Error("submit should not error:", err)
+	}
+
+	// we're using http format so this will have written a whole http request
+	res, err := http.ReadResponse(bufio.NewReader(&out), nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+
+	// {"request":{"echoContent":"yodawg"}}
+	var resp struct {
+		R struct {
+			Body string `json:"echoContent"`
+		} `json:"request"`
+	}
+
+	json.NewDecoder(res.Body).Decode(&resp)
+
+	if resp.R.Body != "yodawg" {
+		t.Fatal(`didn't get a yodawg in the body, http protocol may be fudged up
+			(to debug, recommend ensuring inside the function gets 'Transfer-Encoding: chunked' if
+			no Content-Length is set. also make sure the body makes it (and the image hasn't changed)); GLHF, got:`, resp.R.Body)
+	}
+}

api/agent/protocol/http.go

@@ -3,11 +3,8 @@ package protocol
 import (
 	"bufio"
 	"context"
-	"fmt"
 	"io"
 	"net/http"
-	"net/http/httputil"
-	"strings"
 )
 
 // HTTPProtocol converts stdin/stdout streams into HTTP/1.1 compliant
@@ -26,123 +23,40 @@ func (p *HTTPProtocol) IsStreamable() bool { return true }
 func (h *HTTPProtocol) Dispatch(ctx context.Context, ci CallInfo, w io.Writer) error {
 	req := ci.Request()
 
-	req.RequestURI = ci.RequestURL() // force set to this, for DumpRequestTo to use
+	req.RequestURI = ci.RequestURL() // force set to this, for req.Write to use (TODO? still?)
 
-	err := DumpRequestTo(h.in, req) // TODO timeout
+	// req.Write handles if the user does not specify content length
+	err := req.Write(h.in)
 	if err != nil {
 		return err
 	}
 
-	if rw, ok := w.(http.ResponseWriter); ok {
+	resp, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) // TODO timeout
+	if err != nil {
+		return err
+	}
+	defer resp.Body.Close()
+
+	rw, ok := w.(http.ResponseWriter)
+	if !ok {
+		// async / [some] tests go through here. write a full http request to the writer
+		resp.Write(w) // TODO timeout
+		return nil
+	}
+
 	// if we're writing directly to the response writer, we need to set headers
-		// and status code first since calling res.Write will just write the http
-		// response as the body (headers and all)
+	// and status code, and only copy the body. resp.Write would copy a full
+	// http request into the response body (not what we want).
 
-		res, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) // TODO timeout
-		if err != nil {
-			return err
-		}
-
-		for k, vs := range res.Header {
+	// add resp's on top of any specified on the route [on rw]
+	for k, vs := range resp.Header {
 		for _, v := range vs {
-				rw.Header().Add(k, v) // on top of any specified on the route
+			rw.Header().Add(k, v)
 		}
 	}
-		rw.WriteHeader(res.StatusCode)
-		// TODO should we TCP_CORK ?
-
-		io.Copy(rw, res.Body) // TODO timeout
-		res.Body.Close()
-	} else {
-		// logs can just copy the full thing in there, headers and all.
-
-		res, err := http.ReadResponse(bufio.NewReader(h.out), ci.Request()) // TODO timeout
-		if err != nil {
-			return err
+	if resp.StatusCode > 0 {
+		rw.WriteHeader(resp.StatusCode)
 	}
-		res.Write(w) // TODO timeout
-	}
-
+	io.Copy(rw, resp.Body)
 	return nil
 }
-
-// DumpRequestTo is httputil.DumpRequest with some modifications. It will
-// dump the request to the provided io.Writer with the body always, consuming
-// the body in the process.
-//
-// TODO we should support h2!
-func DumpRequestTo(w io.Writer, req *http.Request) error {
-	// By default, print out the unmodified req.RequestURI, which
-	// is always set for incoming server requests. But because we
-	// previously used req.URL.RequestURI and the docs weren't
-	// always so clear about when to use DumpRequest vs
-	// DumpRequestOut, fall back to the old way if the caller
-	// provides a non-server Request.
-
-	reqURI := req.RequestURI
-	if reqURI == "" {
-		reqURI = req.URL.RequestURI()
-	}
-
-	fmt.Fprintf(w, "%s %s HTTP/%d.%d\r\n", valueOrDefault(req.Method, "GET"),
-		reqURI, req.ProtoMajor, req.ProtoMinor)
-
-	absRequestURI := strings.HasPrefix(req.RequestURI, "http://") || strings.HasPrefix(req.RequestURI, "https://")
-	if !absRequestURI {
-		host := req.Host
-		if host == "" && req.URL != nil {
-			host = req.URL.Host
-		}
-
-		if host != "" {
-			fmt.Fprintf(w, "Host: %s\r\n", host)
-		}
-	}
-
-	chunked := len(req.TransferEncoding) > 0 && req.TransferEncoding[0] == "chunked"
-
-	if len(req.TransferEncoding) > 0 {
-		fmt.Fprintf(w, "Transfer-Encoding: %s\r\n", strings.Join(req.TransferEncoding, ","))
-	}
-
-	if req.Close {
-		fmt.Fprintf(w, "Connection: close\r\n")
-	}
-
-	err := req.Header.WriteSubset(w, reqWriteExcludeHeaderDump)
-	if err != nil {
-		return err
-	}
-
-	io.WriteString(w, "\r\n")
-
-	if req.Body != nil {
-		var dest io.Writer = w
-		if chunked {
-			dest = httputil.NewChunkedWriter(dest)
-		}
-
-		// TODO copy w/ ctx
-		_, err = io.Copy(dest, req.Body)
-		if chunked {
-			dest.(io.Closer).Close()
-			io.WriteString(w, "\r\n")
-		}
-	}
-
-	return err
-}
-
-var reqWriteExcludeHeaderDump = map[string]bool{
-	"Host":              true, // not in Header map anyway
-	"Transfer-Encoding": true,
-	"Trailer":           true,
-}
-
-// Return value if nonempty, def otherwise.
-func valueOrDefault(value, def string) string {
-	if value != "" {
-		return value
-	}
-	return def
-}