Refactor library to use option structs and add better docs/benchmarks

Author: nhooyr

.github/main.workflow

@@ -1,17 +1,21 @@
 workflow "main" {
   on = "push"
-  resolves = ["fmt", "lint", "test"]
+  resolves = ["fmt", "lint", "test", "bench"]
 }
 
 action "lint" {
-  uses = "./.github/lint"
+  uses = "../test/lint"
 }
 
 action "fmt" {
-  uses = "./.github/fmt"
+  uses = "../test/fmt"
 }
 
 action "test" {
-  uses = "./.github/test"
+  uses = "../test/test"
   secrets = ["CODECOV_TOKEN"]
 }
+
+action "bench" {
+  uses = "../test/bench"
+}

.github/test/entrypoint.sh

@@ -1,3 +1,4 @@
 coverage.html
 wstest_reports
 websocket.test
+profs

.gitignore

@@ -4,7 +4,12 @@
 
 websocket is a minimal and idiomatic WebSocket library for Go.
 
-This library is in heavy development.
+At minimum Go 1.12 is required as websocket uses a new [feature](https://github.com/golang/go/issues/26937#issuecomment-415855861) in net/http
+to perform WebSocket handshakes.
+
+This library is not final and the API is subject to change.
+
+If you have any feedback, please feel free to open an issue.
 
 ## Install
 
@@ -15,8 +20,8 @@ go get nhooyr.io/websocket
 ## Features
 
 - Full support of the WebSocket protocol
-- Only depends on stdlib
-- Simple to use
+- Zero dependencies outside of the stdlib
+- Very minimal and carefully considered API
 - context.Context is first class
 - net/http is used for WebSocket dials and upgrades
 - Thoroughly tested, fully passes the [autobahn-testsuite](https://github.com/crossbario/autobahn-testsuite)
@@ -26,9 +31,6 @@ go get nhooyr.io/websocket
 
 - [ ] WebSockets over HTTP/2 [#4](https://github.com/nhooyr/websocket/issues/4)
 - [ ] Deflate extension support [#5](https://github.com/nhooyr/websocket/issues/5)
-- [ ] More optimization [#11](https://github.com/nhooyr/websocket/issues/11)
-- [ ] WASM [#15](https://github.com/nhooyr/websocket/issues/15)
-- [ ] Ping/pongs [#1](https://github.com/nhooyr/websocket/issues/1)
 
 ## Example
 

README.md

@@ -12,43 +12,31 @@ import (
 	"golang.org/x/xerrors"
 )
 
-// AcceptOption is an option that can be passed to Accept.
-// The implementations of this interface are printable.
-type AcceptOption interface {
-	acceptOption()
-}
-
-type acceptSubprotocols []string
-
-func (o acceptSubprotocols) acceptOption() {}
-
-// AcceptSubprotocols lists the websocket subprotocols that Accept will negotiate with a client.
-// The empty subprotocol will always be negotiated as per RFC 6455. If you would like to
-// reject it, close the connection if c.Subprotocol() == "".
-func AcceptSubprotocols(protocols ...string) AcceptOption {
-	return acceptSubprotocols(protocols)
-}
-
-type acceptInsecureOrigin struct{}
-
-func (o acceptInsecureOrigin) acceptOption() {}
-
-// AcceptInsecureOrigin disables Accept's origin verification
-// behaviour. By default Accept only allows the handshake to
-// succeed if the javascript  that is initiating the handshake
-// is on the same domain as the server. This is to prevent CSRF
-// when secure data is stored in cookies.
-//
-// See https://stackoverflow.com/a/37837709/4283659
-//
-// Use this if you want a WebSocket server any javascript can
-// connect to or you want to perform Origin verification yourself
-// and allow some whitelist of domains.
-//
-// Ensure you understand exactly what the above means before you use
-// this option in conjugation with cookies containing secure data.
-func AcceptInsecureOrigin() AcceptOption {
-	return acceptInsecureOrigin{}
+// AcceptOptions represents the options available to pass to Accept.
+type AcceptOptions struct {
+	// Subprotocols lists the websocket subprotocols that Accept will negotiate with a client.
+	// The empty subprotocol will always be negotiated as per RFC 6455. If you would like to
+	// reject it, close the connection if c.Subprotocol() == "".
+	Subprotocols []string
+
+	// InsecureSkipVerify disables Accept's origin verification
+	// behaviour. By default Accept only allows the handshake to
+	// succeed if the javascript that is initiating the handshake
+	// is on the same domain as the server. This is to prevent CSRF
+	// when secure data is stored in a cookie as there is no same
+	// origin policy for WebSockets. In other words, javascript from
+	// any domain can perform a WebSocket dial on an arbitrary server.
+	// This dial will include cookies which means the arbitrary javascript
+	// can perform actions as the authenticated user.
+	//
+	// See https://stackoverflow.com/a/37837709/4283659
+	//
+	// The only time you need this is if your javascript is running on a different domain
+	// than your WebSocket server.
+	// Please think carefully about whether you really need this option before you use it.
+	// If you do, remember if you store secure data in cookies, you wil need to verify the
+	// Origin header.
+	InsecureSkipVerify bool
 }
 
 func verifyClientRequest(w http.ResponseWriter, r *http.Request) error {
@@ -88,26 +76,14 @@ func verifyClientRequest(w http.ResponseWriter, r *http.Request) error {
 // Accept accepts a WebSocket handshake from a client and upgrades the
 // the connection to WebSocket.
 // Accept will reject the handshake if the Origin is not the same as the Host unless
-// the AcceptInsecureOrigin option is passed.
-// Accept uses w to write the handshake response so the timeouts on the http.Server apply.
-func Accept(w http.ResponseWriter, r *http.Request, opts ...AcceptOption) (*Conn, error) {
-	var subprotocols []string
-	verifyOrigin := true
-	for _, opt := range opts {
-		switch opt := opt.(type) {
-		case acceptInsecureOrigin:
-			verifyOrigin = false
-		case acceptSubprotocols:
-			subprotocols = []string(opt)
-		}
-	}
-
+// the InsecureSkipVerify option is set.
+func Accept(w http.ResponseWriter, r *http.Request, opts AcceptOptions) (*Conn, error) {
 	err := verifyClientRequest(w, r)
 	if err != nil {
 		return nil, err
 	}
 
-	if verifyOrigin {
+	if !opts.InsecureSkipVerify {
 		err = authenticateOrigin(r)
 		if err != nil {
 			http.Error(w, err.Error(), http.StatusForbidden)
@@ -127,7 +103,7 @@ func Accept(w http.ResponseWriter, r *http.Request, opts ...AcceptOption) (*Conn
 
 	handleKey(w, r)
 
-	subproto := selectSubprotocol(r, subprotocols)
+	subproto := selectSubprotocol(r, opts.Subprotocols)
 	if subproto != "" {
 		w.Header().Set("Sec-WebSocket-Protocol", subproto)
 	}
@@ -190,5 +166,5 @@ func authenticateOrigin(r *http.Request) error {
 	if strings.EqualFold(u.Host, r.Host) {
 		return nil
 	}
-	return xerrors.Errorf("request origin %q is not authorized", origin)
+	return xerrors.Errorf("request origin %q is not authorized for host %v", origin, r.Host)
 }

accept.go

@@ -12,76 +12,95 @@ import (
 	"nhooyr.io/websocket"
 )
 
-func BenchmarkConn(b *testing.B) {
-	b.StopTimer()
+func benchConn(b *testing.B, stream bool) {
+	name := "buffered"
+	if stream {
+		name = "stream"
+	}
 
-	s, closeFn := testServer(b, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		c, err := websocket.Accept(w, r,
-			websocket.AcceptSubprotocols("echo"),
-		)
-		if err != nil {
-			b.Logf("server handshake failed: %+v", err)
-			return
-		}
-		echoLoop(r.Context(), c)
-	}))
-	defer closeFn()
+	b.Run(name, func(b *testing.B) {
+		s, closeFn := testServer(b, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			c, err := websocket.Accept(w, r, websocket.AcceptOptions{})
+			if err != nil {
+				b.Logf("server handshake failed: %+v", err)
+				return
+			}
+			if stream {
+				streamEchoLoop(r.Context(), c)
+			} else {
+				bufferedEchoLoop(r.Context(), c)
+			}
 
-	wsURL := strings.Replace(s.URL, "http", "ws", 1)
+		}))
+		defer closeFn()
 
-	ctx, cancel := context.WithTimeout(context.Background(), time.Minute*5)
-	defer cancel()
+		wsURL := strings.Replace(s.URL, "http", "ws", 1)
 
-	c, _, err := websocket.Dial(ctx, wsURL)
-	if err != nil {
-		b.Fatalf("failed to dial: %v", err)
-	}
-	defer c.Close(websocket.StatusInternalError, "")
+		ctx, cancel := context.WithTimeout(context.Background(), time.Minute*5)
+		defer cancel()
 
-	runN := func(n int) {
-		b.Run(strconv.Itoa(n), func(b *testing.B) {
-			msg := []byte(strings.Repeat("2", n))
-			buf := make([]byte, len(msg))
-			b.SetBytes(int64(len(msg)))
-			b.ResetTimer()
-			for i := 0; i < b.N; i++ {
-				w, err := c.Write(ctx, websocket.MessageText)
-				if err != nil {
-					b.Fatal(err)
-				}
+		c, _, err := websocket.Dial(ctx, wsURL, websocket.DialOptions{})
+		if err != nil {
+			b.Fatalf("failed to dial: %v", err)
+		}
+		defer c.Close(websocket.StatusInternalError, "")
 
-				_, err = w.Write(msg)
-				if err != nil {
-					b.Fatal(err)
-				}
+		runN := func(n int) {
+			b.Run(strconv.Itoa(n), func(b *testing.B) {
+				msg := []byte(strings.Repeat("2", n))
+				buf := make([]byte, len(msg))
+				b.SetBytes(int64(len(msg)))
+				b.ResetTimer()
+				for i := 0; i < b.N; i++ {
+					if stream {
+						w, err := c.Writer(ctx, websocket.MessageText)
+						if err != nil {
+							b.Fatal(err)
+						}
 
-				err = w.Close()
-				if err != nil {
-					b.Fatal(err)
-				}
+						_, err = w.Write(msg)
+						if err != nil {
+							b.Fatal(err)
+						}
 
-				_, r, err := c.Read(ctx)
-				if err != nil {
-					b.Fatal(err, b.N)
-				}
+						err = w.Close()
+						if err != nil {
+							b.Fatal(err)
+						}
+					} else {
+						err = c.Write(ctx, websocket.MessageText, msg)
+						if err != nil {
+							b.Fatal(err)
+						}
+					}
+					_, r, err := c.Reader(ctx)
+					if err != nil {
+						b.Fatal(err, b.N)
+					}
 
-				_, err = io.ReadFull(r, buf)
-				if err != nil {
-					b.Fatal(err)
+					_, err = io.ReadFull(r, buf)
+					if err != nil {
+						b.Fatal(err)
+					}
 				}
-			}
-			b.StopTimer()
-		})
-	}
+				b.StopTimer()
+			})
+		}
+
+		runN(32)
+		runN(128)
+		runN(512)
+		runN(1024)
+		runN(4096)
+		runN(16384)
+		runN(65536)
+		runN(131072)
 
-	runN(32)
-	runN(128)
-	runN(512)
-	runN(1024)
-	runN(4096)
-	runN(16384)
-	runN(65536)
-	runN(131072)
+		c.Close(websocket.StatusNormalClosure, "")
+	})
+}
 
-	c.Close(websocket.StatusNormalClosure, "")
+func BenchmarkConn(b *testing.B) {
+	benchConn(b, true)
+	benchConn(b, false)
 }

bench_test.go

@@ -0,0 +1,10 @@
+FROM golang:1.12
+
+LABEL "com.github.actions.name"="bench"
+LABEL "com.github.actions.description"="bench"
+LABEL "com.github.actions.icon"="code"
+LABEL "com.github.actions.color"="purple"
+
+COPY entrypoint.sh /entrypoint.sh
+
+CMD ["/entrypoint.sh"]

ci/bench/Dockerfile

@@ -0,0 +1,15 @@
+#!/usr/bin/env bash
+
+source ci/lib.sh || exit 1
+
+mkdir -p profs
+go test --vet=off --run=^$ -bench=. \
+	./...
+#	-cpuprofile=profs/cpu \
+#	-memprofile=profs/mem \
+#	-blockprofile=profs/block \
+#	-mutexprofile=profs/mutex \
+
+set +x
+echo "profiles are in ./profs
+keep in mind that every profiler Go provides is enabled so that may skew the benchmarks"

ci/bench/entrypoint.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-source .github/lib.sh || exit 1
+source ci/lib.sh || exit 1
 
 gen() {
 	# Unfortunately, this is the only way to ensure go.mod and go.sum are correct.