-2

I'm writing an HTTP API that sends requests to a WebSocket gateway. I'm using go1.14.7 and gorilla/mux v1.8.0. The code will be cross-compiled using GOOS=linux GOARCH=arm GOARM=7.

My problems are the following:

  • respondBadRequest() always logs this even though I never use WriteHeader:
    • http: superfluous response.WriteHeader call from main.(*HttpHandler).respondBadRequest
  • The API response is always 200 OK even when respondBadRequest() is called
  • The API response always has an empty body

I'm completely new to Go. Below is my code structure.

type HttpHandler struct {
  gateway Gateway
}

func (h *HttpHandler) respondSuccess(w http.ResponseWriter, text string) {
  w.Write([]byte(text))
}

func (h *HttpHandler) respondBadRequest(w http.ResponseWriter, text string) {
  http.Error(w, text, http.StatusBadRequest)
}

func (h *HttpHandler) respondError(w http.ResponseWriter, text string) {
  http.Error(w, text, http.StatusInternalServerError)
}

func (h *HttpHandler) OnFoobar(w http.ResponseWriter, r *http.Request) {
  f := func(success bool, e error) {
    if e != nil {
      h.respondError(w, e.Error())
    } else if success {
      h.respondSuccess(w, "Foobar Accepted")
    } else {
      h.respondBadRequest(w, "Unknown Foobar")
    }
  }
  //...
  e := h.gateway.Foobar(f)
  if e != nil {
    log.Println(e)
  }
}

//...

httpHandler := &HttpHandler{
  gateway: gateway,
}

r := mux.NewRouter()

r.HandleFunc("/foobar", httpHandler.OnFoobar)

http.ListenAndServe(":8000", r)
Steffen
  • 1,328
  • 3
  • 12
  • 29
  • 2
    If you write any part of a response, `WriteHeader` is already called. You can't send a different header after you've already written a response. – JimB Mar 16 '21 at 16:00
  • 3
    You're not calling `WriteHeader` directly, but you're calling it indirectly in several places; if you call `Write` and `WriteHeader` hasn't already been called, `Write` will call `WriteHeader(200)` (as stated in the docs). `http.Error` also calls `WriteHeader`. If you want to respond with an error, you have to do so *before* calling `Write` to send any body content (in an HTTP response, the status and headers *must* precede the response body). – Adrian Mar 16 '21 at 16:05
  • Thanks for the replies, but I don't really understand what you mean. I'm aware that any `Write` will call `WriteHeader` since I tried to debug my issue before posting here. My issue is that I don't understand where that first `Write` happens that makes mine superfluous. Apparently in `gateway.Foobar()`, but I can't tell why or how to restructure this so that it doesn't interfere with my surrounding API code. – Steffen Mar 16 '21 at 16:20
  • 1
    `f` looks like a callback, so I guess `Foobar` is asynchronous? Is it doing it's job in a goroutine? If so the handler `OnFoobar` does not wait for `Foobar` to finish and when it exits, if `w` hasn't been written to yet, Go's serve mux will write to it 200ok by default, then later when `Foobar`'s done and it invokes the callback you get the error you get. – mkopriva Mar 16 '21 at 16:36
  • @mkopriva Yes, it's asynchronous and thanks for actually understanding the issue... Now that you mention it, it does seem obvious. I guess I could use a channel to delay `OnFinish()` until `f()` is done. Or is there a smarter way around this? – Steffen Mar 16 '21 at 16:48
  • @Steffen that seems like unnecessary complexity to me, why don't you remove the callback, make Foobar synchronous, have it return an error that can be inspected to then decide upon the proper status code. Note that in Go the standard router, and I'm pretty sure gorilla/mux as well, already handle each request in a separate goroutine, so you don't have to roll your own asynchronicity. – mkopriva Mar 16 '21 at 16:52
  • @mkopriva Unfortunately `Foobar` is from a third-party package I have no control over. – Steffen Mar 16 '21 at 16:55
  • 1
    @Steffen in that case use a `sync.WaitGroup`. – mkopriva Mar 16 '21 at 16:56
  • 1
    @Steffen something like this should do, I believe: https://play.golang.org/p/_2DkCVXmhcS – mkopriva Mar 16 '21 at 17:06
  • 1
    @mkopriva I tried a channel and it works. Thanks so much. I have a look at `sync.WaitGroup` and answer my (apparently terrible...) question later. – Steffen Mar 16 '21 at 17:07

1 Answers1

1

So as @mkopriva pointed out f is rather obviously called after OnFoobar already finished and wrote an implicit empty response. His fix using sync.WaitGroup:

func (h *HttpHandler) OnFoobar(w http.ResponseWriter, r *http.Request) {
  wg := new(sync.WaitGroup)
  wg.Add(1)
  f := func(success bool, e error) {
    if e != nil {
      h.respondError(w, e.Error())
    } else if success {
      h.respondSuccess(w, "Foobar Accepted")
    } else {
      h.respondBadRequest(w, "Unknown Foobar")
    }
    wg.Done()
  }
  //...
  e := h.gateway.Foobar(f)
  if e != nil {
    log.Println(e)
  }
  wg.Wait()
}
E_net4
  • 27,810
  • 13
  • 101
  • 139
Steffen
  • 1,328
  • 3
  • 12
  • 29