Do not allocate empty response in case of error #204
Conversation
client.go
Outdated
| @@ -185,7 +185,7 @@ func (c *Client) PushWithContext(ctx Context, n *Notification) (*Response, error | |||
|
|
|||
| decoder := json.NewDecoder(httpRes.Body) | |||
| if err := decoder.Decode(&response); err != nil && err != io.EOF { | |||
| return &Response{}, err | |||
| return nil, err | |||
There was a problem hiding this comment.
Thanks for the PR. This case is a little different as we've already completed the http request and have headers back etc, but just cant parse the json body, presumably because Apple returned us corrupt json. So in effect the err would be pretty useless to the user as it would be a json decoding err, and the response (even though it only contains a status code and an apns-id may be of more use to them. Semantically, I probably should change it so it returns either a response or an error and not both.
I'm looking to review this error handling in the future to read the error from the GOAWAY frame as it contains useful information.
There was a problem hiding this comment.
I believe it's very unlikely situation, anyway, I am more concerned about backward compatibility of this change, so it's up to you.
There was a problem hiding this comment.
We can wrap error with fmt.Errorf("apn: info from header probably or readed response, %w", err) for this case or wrap with custom error
moredure
force-pushed
the
patch-4
branch
10 times, most recently
from
ec1a5c1
to
c23618a
Compare
Doc string states that it's return response in case of success or an error in case of error
Doc string states that it's return response in case of success or an error in case of error