Skip to content
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
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
45 changes: 45 additions & 0 deletions pkg/webservice/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
package webservice

import (
"compress/gzip"
"encoding/json"
"fmt"
"io"
Expand Down Expand Up @@ -59,6 +60,7 @@ const (
GroupNameMissing = "Group name is missing"
ApplicationDoesNotExists = "Application not found"
NodeDoesNotExists = "Node not found"
UnsupportedCompType = "Compression type not support"
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: "not supported"

Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't be here at all. As I've repeatedly indicated we should never fail with an error, just fall back to no compression.

)

var allowedActiveStatusMsg string
Expand Down Expand Up @@ -153,6 +155,10 @@ func writeHeaders(w http.ResponseWriter) {
w.Header().Set("Access-Control-Allow-Headers", "X-Requested-With,Content-Type,Accept,Origin")
}

func writeHeader(w http.ResponseWriter, key, val string) {
w.Header().Set(key, val)
}

func buildJSONErrorResponse(w http.ResponseWriter, detail string, code int) {
w.WriteHeader(code)
errorInfo := dao.NewYAPIError(nil, code, detail)
Expand Down Expand Up @@ -745,6 +751,10 @@ func getQueueApplications(w http.ResponseWriter, r *http.Request) {
for _, app := range queue.GetCopyOfApps() {
appsDao = append(appsDao, getApplicationDAO(app))
}
if checkHeader(r.Header, "Accept-Encoding", "gzip") {
compress(w, appsDao)
return
}

if err := json.NewEncoder(w).Encode(appsDao); err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
Expand Down Expand Up @@ -1216,3 +1226,38 @@ func getStream(w http.ResponseWriter, r *http.Request) {
}
}
}

func checkHeader(h http.Header, key string, value string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Should we return error if users use unsupported compression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This may not be necessary. Skipping it is a solution when dealing with unsupported compression types in requests. Do you think it's essential for users to require this one?

Copy link
Member

Choose a reason for hiding this comment

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

Please take a look https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/406

It seems to me following the standard error can avoid the misunderstanding in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me following the standard error can avoid the misunderstanding in the future.

NO. We should never return an error to the client if it request an encoding we don't understand. Accept-Encoding: identity is the default, which means the identity encoding is always allowed. Therefore, if an unacceptable encoding is requested, we simply send the request uncompressed and without a Content-Encoding: gzip header.

Copy link
Member

Choose a reason for hiding this comment

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

NO. We should never return an error to the client if it request an encoding we don't understand. Accept-Encoding: identity is the default, which means the identity encoding is always allowed. Therefore, if an unacceptable encoding is requested, we simply send the request uncompressed and without a Content-Encoding: gzip header.

That is an acceptable way to me, but I'd like to have more discussion for my own education :)

Should we support full representation of Accept-Encoding ( weight and coding )? If yes, we need to consider the Accept-Encoding: gzip;q=1.0, identity; q=0.

Or we ignore the weight and only check the existence of gzip from the Accept-Encoding. This is the solution adopted by this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a common saying in network protocol design: Be liberal in what you accept, strict in what you produce. In other words, we can get away with just checking for the substring gzip in Accept-Encoding, and produce exactly Content-Encoding: gzip in that case. If we choose not to compress due to size, or gzip was not requested, then we use the standard identity version. Weights are not really necessary; yes, they are part of the spec, but the client is only giving its preference; we do not have to honor it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to see a very simple function like: GetCompressedWriter(headers, writer) (writer) that checks for the gzip header and wraps the given writer with a gzip-compressed one, else returns the original writer. Then in any endpoint we want to (potentially compress), we just replace our writer with that one instead.

Copy link
Contributor

@wilfred-s wilfred-s Mar 20, 2024

Choose a reason for hiding this comment

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

That will cause a leak as the gzip writer must be closed for it not to leak. Calling close on the http.ResponseWriter is not possible so we need more. Probably the easiest solution is to use the same solution as we have for the loggingHandler(). We wrap the compression choice in a handler function, which then gets wrapped in the logging handler. That means we have it all in one place and expand on it with compressor pooling or other things in the future.

Example code, which is not complete but gives some idea on how we can close the compressor. That can be expanded to use a sync.Pool to not recreate the zip writer each time and just reset it before use.

type gzipResponseWriter struct {
	io.Writer
	http.ResponseWriter
}

func (w gzipResponseWriter) Write(b []byte) (int, error) {
	return w.Writer.Write(b)
}

func makeGzipHandler(fn http.HandlerFunc) http.HandlerFunc {
	return func(w http.ResponseWriter, r *http.Request) {
		if !strings.Contains(r.Header.Get("Accept-Encoding"), "gzip") {
			fn(w, r)
			return
		}
		w.Header().Set("Content-Encoding", "gzip")
                w.Header().Del("Content-Length")
		gz := gzip.NewWriter(w)
		defer gz.Close()
		gzr := gzipResponseWriter{Writer: gz, ResponseWriter: w}
		fn(gzr, r)
	}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to everyone for all the advice. I have got some questions about what @wilfred-s said.
In the provided example, the compress handler would be wrapped within the logging handler. So we first see which API is being called, and then decide whether to use the gzip handler, since we haven't decided to compress all APIs yet? Is there any misunderstanding?

values := h.Values(key)
for _, v := range values {
if strings.Contains(v, value) {
return true
}
}
return false
}

func compress(w http.ResponseWriter, data any) {
response, err := json.Marshal(data)
if err != nil {
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
return
}

writer := gzip.NewWriter(w)
Copy link
Contributor

Choose a reason for hiding this comment

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

close the writer with a defer call eg. (defer writer.Close() or similar)

w.Header().Set("Content-Encoding", "gzip")
_, err = writer.Write(response)
if err != nil {
_ = writer.Close()
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
return
}
err = writer.Flush()
if err != nil {
_ = writer.Close()
buildJSONErrorResponse(w, err.Error(), http.StatusInternalServerError)
return
}

_ = writer.Close()
}
24 changes: 24 additions & 0 deletions pkg/webservice/handlers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,13 @@
package webservice

import (
"bytes"
"compress/gzip"
"context"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
"reflect"
Expand Down Expand Up @@ -1469,6 +1472,27 @@ func TestGetQueueApplicationsHandler(t *testing.T) {
resp = &MockResponseWriter{}
getQueueApplications(resp, req)
assertParamsMissing(t, resp)

// test compression
var req4 *http.Request
req4, err = http.NewRequest("GET", "/ws/v1/partition/default/queue/root.default/applications", strings.NewReader(""))
req4 = req4.WithContext(context.WithValue(req.Context(), httprouter.ParamsKey, httprouter.Params{
httprouter.Param{Key: "partition", Value: partitionNameWithoutClusterID},
httprouter.Param{Key: "queue", Value: "root.default"},
}))
req4.Header.Set("Accept-Encoding", "gzip")
assert.NilError(t, err, "Get Queue Applications Handler request failed")
resp4 := &MockResponseWriter{}
getQueueApplications(resp4, req4)

var appsDao4 []*dao.ApplicationDAOInfo
buf := bytes.NewBuffer(resp4.outputBytes)
gzipReader, err := gzip.NewReader(buf)
assert.NilError(t, err, "Create gzip reader failed.")
uncompress, err := io.ReadAll(gzipReader)
assert.NilError(t, err, "Decode gzip data failed.")
err = json.Unmarshal(uncompress, &appsDao4)
assert.NilError(t, err, unmarshalError)
}

func checkLegalGetAppsRequest(t *testing.T, url string, params httprouter.Params, expected []*dao.ApplicationDAOInfo) {
Expand Down