matrix-org / gomatrix

A Golang Matrix client
Apache License 2.0
285 stars 53 forks source link

Streaming JSON encoding/decoding via HTTP client #66

Closed ladyisatis closed 5 years ago

ladyisatis commented 5 years ago

The current method, for its HTTP client, allocates a byte array, marshaled into the byte array, and then wraps that into a byte buffer. Similarly, the entire HTTP Response body is read into memory, then unmarshaled, and then parsed. This might become an issue because of excessive memory if there's a large amount of JSON being received from any particular Matrix server, so this PR skips a few steps and uses the encoding/json NewEncoder() to encode the interface directly to a byte buffer, and NewDecoder() for the io.ReadCloser from the response. This lowers memory usage and any unnecessary swapping.

Downside to this is that the contents of the JSON file cannot be passed via return variable, though I'm not sure the value in returning the contents with a 2XX response, if it is being unmarshaled anyway? In lieu of this, if it isn't a 2XX response, then the contents will be read into memory, as the amount of memory needed is low for returned errors, and included in the HTTPError struct. As a consequence, it becomes a breaking change for any applications that rely on the MakeRequest function.

ladyisatis commented 5 years ago

By the way - the mock HTTP client seems to be nonfunctional? From master branch:

$ go test
--- FAIL: TestClient_GetAvatarUrl (0.00s)
    client_test.go:41: GetAvatarURL: Got empty response
--- FAIL: TestClient_StateEvent (0.00s)
    client_test.go:83: StateEvent: got , want Room Name Goes Here
FAIL
exit status 1
babolivier commented 5 years ago

And obviously, thank you very much for your contribution! :)

spantaleev commented 5 years ago

I remember using the body bytes being helpful during development and good for throwing away after that, as the code was getting polished.

Also, with the new way the response contents are passed to the error, it's harder to figure out what's wrong if one simply logs the error as is. You now need to explicitly dig into err.Contents to decode it and log it in a friendlier format.

It's not to say that I advocated for reverting this change. I just thought it'd be nice to point these things out.