Skip to content

Commit

Permalink
Set some server http timeouts
Browse files Browse the repository at this point in the history
When using GoatCounter directly internet-facing it's liable to keep
connections around for far too long, exhausting the max. number of open
file descriptors, especially with "idle" HTTP/2 connections which,
unlike HTTP/1.1 Keep-Alive don't have an explicit timeout.

This isn't much of a problem if you're using a proxy in front of it, as
most will have some timeouts set by default (unlike Go, which has no
timeouts at all by default).

For the backend interface, keeping a long timeout makes sense; it
reduces overhead on requests (TLS setup alone can be >200ms), but for
the /count request we typically want a much shorter timeout.

Unfortunately, configuring timeouts per-endpoint isn't really supported
at this point, although some possible workarounds are mentioned in [1],
it's all pretty ugly.
We can add "Connection: close" to just close the connection, which is
probably much better for almost all cases than keeping a connection open
since most people only visit a single page, and keeping a connection
open in the off-chance they click somewhere again probably isn't really
worth it.

And setting *any* timeout is better than setting *no* timeout at all!

---

In the email conversation about this, the other person mentioned that
some connections linger for hours, so there *may* be an additional
problem either in the Go library or in GoatCounter, since this is a
really long time for a connection to stay open. Then again, it could
also be weird scripts or malicious users 🤔

[1]: golang/go#16100
  • Loading branch information
arp242 committed Jul 17, 2020
1 parent 8f499d8 commit 9d36f1b
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 1 deletion.
7 changes: 7 additions & 0 deletions cmd/goatcounter/saas.go
Expand Up @@ -172,6 +172,13 @@ func saas() (int, error) {
Addr: listen,
Handler: zhttp.HostRoute(hosts),
TLSConfig: tlsc,

// Set some reasonably high timeouts which should never be reached.
// Note that handlers have a 5-second timeout set in handlers/mw.go
ReadHeaderTimeout: 10 * time.Second,
ReadTimeout: 60 * time.Second,
WriteTimeout: 60 * time.Second,
IdleTimeout: 120 * time.Second,
}, func() {
zlog.Printf("serving %q on %q; dev=%t", cfg.Domain, listen, dev)
})
Expand Down
8 changes: 8 additions & 0 deletions cmd/goatcounter/serve.go
Expand Up @@ -8,6 +8,7 @@ import (
"context"
"net/http"
"strings"
"time"

"zgo.at/goatcounter"
"zgo.at/goatcounter/acme"
Expand Down Expand Up @@ -165,6 +166,13 @@ func serve() (int, error) {
Addr: listen,
Handler: zhttp.HostRoute(hosts),
TLSConfig: tlsc,

// Set some reasonably high timeouts which should never be reached.
// Note that handlers have a 5-second timeout set in handlers/mw.go
ReadHeaderTimeout: 10 * time.Second,
ReadTimeout: 60 * time.Second,
WriteTimeout: 60 * time.Second,
IdleTimeout: 120 * time.Second,
}, func() {
banner()
zlog.Printf("ready; serving %d sites on %q; dev=%t; sites: %s",
Expand Down
8 changes: 7 additions & 1 deletion handlers/backend.go
Expand Up @@ -209,8 +209,14 @@ func (h backend) count(w http.ResponseWriter, r *http.Request) error {
w.Header().Set("Access-Control-Allow-Origin", "*")
w.Header().Set("Content-Type", "image/gif")

bot := isbot.Bot(r)
// Note this works in both HTTP/1.1 and HTTP/2, as the Go HTTP/2 server
// picks up on this and sends the GOAWAY frame.
// TODO: it would be better to set a short idle timeout, but this isn't
// really something that can be configured per-handler at the moment.
// https://github.com/golang/go/issues/16100
w.Header().Set("Connection", "close")

bot := isbot.Bot(r)
// Don't track pages fetched with the browser's prefetch algorithm.
if bot == isbot.BotPrefetch {
return zhttp.Bytes(w, gif)
Expand Down

0 comments on commit 9d36f1b

Please sign in to comment.