From 94cca0adca8015e361ae9554b98a8ed19d11bcef Mon Sep 17 00:00:00 2001 From: diamondburned Date: Tue, 4 Aug 2020 14:09:37 -0700 Subject: [PATCH] httputil: Fixed unlock of unlocked mutex bug --- api/rate/rate.go | 5 +++-- internal/moreatomic/mutex.go | 10 ++++++++++ utils/httputil/client.go | 38 +++++++++++++++++++++++++++--------- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/api/rate/rate.go b/api/rate/rate.go index b446197..4cf7e57 100644 --- a/api/rate/rate.go +++ b/api/rate/rate.go @@ -128,14 +128,15 @@ func (l *Limiter) Acquire(ctx context.Context, path string) error { } // Release releases the URL from the locks. This doesn't need a context for -// timing out, it doesn't block that much. +// timing out, since it doesn't block that much. func (l *Limiter) Release(path string, headers http.Header) error { b := l.getBucket(path, false) if b == nil { return nil } - defer b.lock.Unlock() + // TryUnlock because Release may be called when Acquire has not been. + defer b.lock.TryUnlock() // Check custom limiter if b.custom != nil { diff --git a/internal/moreatomic/mutex.go b/internal/moreatomic/mutex.go index 3264ac9..0031141 100644 --- a/internal/moreatomic/mutex.go +++ b/internal/moreatomic/mutex.go @@ -42,6 +42,16 @@ func (m *CtxMutex) Lock(ctx context.Context) error { } } +// TryUnlock returns true if the mutex has been unlocked. +func (m *CtxMutex) TryUnlock() bool { + select { + case <-m.mut: + return true + default: + return false + } +} + func (m *CtxMutex) Unlock() { select { case <-m.mut: diff --git a/utils/httputil/client.go b/utils/httputil/client.go index 06826f3..1b15db3 100644 --- a/utils/httputil/client.go +++ b/utils/httputil/client.go @@ -67,19 +67,22 @@ func (c *Client) Context() context.Context { return c.context } -func (c *Client) applyOptions(r httpdriver.Request, extra []RequestOption) error { +// applyOptions tries to apply all options. It does not halt if a single option +// fails, and the error returned is the latest error. +func (c *Client) applyOptions(r httpdriver.Request, extra []RequestOption) (e error) { for _, opt := range c.OnRequest { if err := opt(r); err != nil { - return err - } - } - for _, opt := range extra { - if err := opt(r); err != nil { - return err + e = err } } - return nil + for _, opt := range extra { + if err := opt(r); err != nil { + e = err + } + } + + return } func (c *Client) MeanwhileMultipart( @@ -158,6 +161,8 @@ func (c *Client) Request(method, url string, opts ...RequestOption) (httpdriver. var r httpdriver.Response var status int + // The c.Retries < 1 check ensures that we retry forever if that field is + // less than 1. for i := uint(0); c.Retries < 1 || i < c.Retries; i++ { q, err := c.Client.NewRequest(c.context, method, url) if err != nil { @@ -165,18 +170,33 @@ func (c *Client) Request(method, url string, opts ...RequestOption) (httpdriver. } if err := c.applyOptions(q, opts); err != nil { + // We failed to apply an option, so we should call all OnResponse + // handler to clean everything up. + for _, fn := range c.OnResponse { + fn(q, nil) + } + // Exit after cleaning everything up. return nil, errors.Wrap(err, "failed to apply options") } r, doErr = c.Client.Do(q) + // Error that represents the latest error in the chain. + var onRespErr error + // Call OnResponse() even if the request failed. for _, fn := range c.OnResponse { + // Be sure to call ALL OnResponse handlers. if err := fn(q, r); err != nil { - return nil, err + onRespErr = err } } + if onRespErr != nil { + return nil, errors.Wrap(err, "OnResponse handler failed") + } + + // Retry if the request failed. if doErr != nil { continue }