Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server/writer: properly handle result encoding errors #7114

Merged
merged 2 commits into from
Oct 15, 2024
Merged

server/writer: properly handle result encoding errors #7114

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2bf9898
server/writer: don't call WriteStatus() twice on encoding errors
srenatus Oct 11, 2024
e1aca82
server/writer: use Header().Set() not Header().Add() to avoid duplica…
srenatus Oct 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions server/writer/writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,21 +49,23 @@ func ErrorString(w http.ResponseWriter, status int, code string, err error) {

// Error writes a response with specified status and error response.
func Error(w http.ResponseWriter, status int, err *types.ErrorV1) {
headers := w.Header()
headers.Add("Content-Type", "application/json")
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(status)
_, _ = w.Write(append(err.Bytes(), byte('\n')))
}

// JSON writes a response with the specified status code and object. The object
// will be JSON serialized.
// Deprecated: This method is problematic when using a non-200 status `code`: if
// encoding the payload fails, it'll print "superfluous call to WriteHeader()"
// logs.
func JSON(w http.ResponseWriter, code int, v interface{}, pretty bool) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only a single caller to this method, and it's the health check logging an error. The payloads involved with that are so simple that this problem shouldn't really apply -- but we could refactor this nonetheless.

enc := json.NewEncoder(w)
if pretty {
enc.SetIndent("", " ")
}

w.Header().Add("Content-Type", "application/json")
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(code)

if err := enc.Encode(v); err != nil {
Expand All @@ -74,7 +76,18 @@ func JSON(w http.ResponseWriter, code int, v interface{}, pretty bool) {

// JSONOK is a helper for status "200 OK" responses
func JSONOK(w http.ResponseWriter, v interface{}, pretty bool) {
JSON(w, http.StatusOK, v, pretty)
enc := json.NewEncoder(w)
if pretty {
enc.SetIndent("", " ")
}

w.Header().Add("Content-Type", "application/json")
// If Encode() calls w.Write() for the first time, it'll set the HTTP status
// to 200 OK.
if err := enc.Encode(v); err != nil {
ErrorAuto(w, err)
return
}
}

// Bytes writes a response with the specified status code and bytes.
Expand Down