Switch to json.Unmarshal instead of json.Decoder.Decode() #174
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is replaced for a few reasons,
- The JSON stream decoder blocks forever if the server stops sending data
midway in the body, see goroutine dump below [0].
- Switching this to the json.Marshal surfaced issues in implementations where
multiple JSON blocks were being returned in the response to the client,
e.g: {"foo": "bar"}{"foo": "baz"}
they are not caught in tests because json.Decoder.Decode doesn't have a problem with those.
- As of now this service does not stream JSON and if the need arises,
the implementation could be switched back to the json.Decoder along
with a for, select loop on the request context.
[0]. Goroutine hung on json.(*Decoder).Decode
```
1 @ 0x43b196 0x44abf2 0x6bf129 0x6be71e 0x46ba21
0x6bf128 net/http.(*http2clientStream).writeRequest+0x9c8
/opt/hostedtoolcache/go/1.18.5/x64/src/net/http/h2_bundle.go:8037
0x6be71d net/http.(*http2clientStream).doRequest+0x1d
/opt/hostedtoolcache/go/1.18.5/x64/src/net/http/h2_bundle.go:7899
1 @ 0x43b196 0x467afd 0x467add 0x48024c 0x6a9aab 0x6c5be5 0x56ee5f 0x56ea5b
0x56e798 0xb627dc 0xb60de7 0xb64065 0xcd3f85 0xcd30dc 0xcd2ba5 0xd128fe
0x46ba21
0x467adc sync.runtime_notifyListWait+0x11c
/opt/hostedtoolcache/go/1.18.5/x64/src/runtime/sema.go:513
0x48024b sync.(*Cond).Wait+0x8b
/opt/hostedtoolcache/go/1.18.5/x64/src/sync/cond.go:56
0x6a9aaa net/http.(*http2pipe).Read+0xea
/opt/hostedtoolcache/go/1.18.5/x64/src/net/http/h2_bundle.go:3663
0x6c5be4 net/http.http2transportResponseBody.Read+0x84
/opt/hostedtoolcache/go/1.18.5/x64/src/net/http/h2_bundle.go:9098
0x56ee5e encoding/json.(*Decoder).refill+0x17e
/opt/hostedtoolcache/go/1.18.5/x64/src/encoding/json/stream.go:165
0x56ea5a encoding/json.(*Decoder).readValue+0xba
/opt/hostedtoolcache/go/1.18.5/x64/src/encoding/json/stream.go:140
0x56e797 encoding/json.(*Decoder).Decode+0x77
/opt/hostedtoolcache/go/1.18.5/x64/src/encoding/json/stream.go:63
```
In order to reduce ambiguity in who writes the response body, this method now just returns an error if any. and leaves it to the caller to write the response based on the error. Earlier this method was writing a JSON response to the body, which led to multiple situations where the JSON response was written again by the caller. And so invalid JSON being sent in the JSON body.
Codecov Report
@@ Coverage Diff @@
## main #174 +/- ##
==========================================
- Coverage 72.44% 71.27% -1.18%
==========================================
Files 35 35
Lines 3321 3380 +59
==========================================
+ Hits 2406 2409 +3
- Misses 673 721 +48
- Partials 242 250 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
sfunkhouser
previously approved these changes
Dec 5, 2022
JAORMX
suggested changes
Dec 7, 2022
| return err | ||
| } | ||
|
|
||
| if err := json.Unmarshal(data, &se); err != nil { |
There was a problem hiding this comment.
Given this is a change that could easily be regressed, could you add unit tests that ensure that we don't fall back to the previous behavior? I can very easily see that someone is not aware of the Unmarshall/Decode issues and moves back to using a decoder.
JAORMX
approved these changes
Dec 7, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is replaced for a few reasons,
The JSON stream decoder blocks forever if the server stops sending data
midway in the body, see goroutine dump below [0].
Switching this to the json.Marshal surfaced issues in implementations where
multiple JSON blocks were being returned in the response to the client,
e.g:
{"foo": "bar"}{"foo": "baz"}they are not caught in tests because json.Decoder.Decode doesn't have a problem with those.
As of now this service does not stream JSON and if the need arises,
the implementation could be switched back to the json.Decoder along
with a for, select loop on the request context.
[0]. Goroutine hung on json.(*Decoder).Decode