search

zmb3 / spotify

A Go wrapper for the Spotify Web API
Apache License 2.0
1.33k stars 284 forks source link

Remove 202 from shouldRetry #183

Closed Wakeful-Cloud closed 1 year ago

Wakeful-Cloud commented 2 years ago

Fixes #182

NOTE: I don't know if http.StatusAccepted is used by other endpoints in cases where the response needs to be retried.

strideynet commented 2 years ago

Thanks for raising this. I'll take a look tonight along with the other open PR.

strideynet commented 2 years ago

I'm a little worried this might break some existing retries that are valid, So I'm going to need to take a bit of a deeper look.

Wakeful-Cloud commented 2 years ago

@strideynet That was my concern too. I wonder if it's possible to analyze a response header/body to determine if the request actually failed or not. Are you aware of any known cases where Spotify returns 202 even though the request should be retried?

rtrox commented 1 year ago

I was planning on addressing this too, then saw this PR. I think given that StatusAccepted was added for a reason, a safer approach might be to instead change the retry checks in c.execute and c.get to include a check of the needsStatus:

if c.AutoRetry &&
   isFailure(resp.StatusCode, needsStatus) &&
   shouldRetry(resp.StatusCode) {
   // <retry logic>
}
rtrox commented 1 year ago

gah, sorry, just realized I'm necro-ing a year old PR. will open a new PR for this.

Wakeful-Cloud commented 1 year ago

213 seems more complete, closing