common/lazyslot: Simplify code a little bit.

common/lazyslot/lazyslot.go

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 // Package lazyslot implements a caching scheme for globally shared objects that
-// take significant time to refresh. The defining property of the implementation
-// is that only one goroutine (can be background one) will block when refreshing
-// such object, while all others will use a slightly stale cached copy.
+// take significant time to refresh.
+//
+// The defining property of the implementation is that only one goroutine
+// (can be background one) will block when refreshing such object, while all
+// others will use a slightly stale cached copy.
 package lazyslot
 
 import (
+        "fmt"
         "sync"
         "time"
 
         "golang.org/x/net/context"
 
         "github.com/luci/luci-go/common/clock"
 )
 
 // Value is what's stored in a Slot. It is treated as immutable value.
 type Value struct {
         // Value is whatever fetcher returned.
         Value interface{}
         // Expiration is time when this value expires and should be refetched.
         Expiration time.Time
 }
 
-// Fetcher knows how to load new value.
+// Fetcher knows how to load a new value.
+//
+// If it returns no errors, it MUST return non-nil Value.Value or Slot.Get will
+// return error.
+//
+// Panics inside Fetcher will be caught and converted to errors.

[iannucci, 2016/03/01 00:42:41]

ugh.. can we just let panics crash the program? I don't think we should be in
the habit of silent panic->error conversions. If the Fetcher implementor wanted
all panics to get converted to errors, they should write that in their
implementation.

[Vadim Sh., 2016/03/01 01:11:26]

Okay. It used to work like that. Reverted.

[dnj, 2016/03/01 01:21:45]

+1

 type Fetcher func(c context.Context, prev Value) (Value, error)
 
-// Slot holds a cached Value and refreshes it when it expires. Only one
-// goroutine will be busy refreshing, all others will see a slightly stale
-// copy of the value during the refresh.
+// Slot holds a cached Value and refreshes it when it expires.
+//
+// Only one goroutine will be busy refreshing, all others will see a slightly
+// stale copy of the value during the refresh.
 type Slot struct {
         Fetcher Fetcher       // used to actually load the value on demand
         Timeout time.Duration // how long to allow to fetch, 5 sec by default.
         Async   bool          // if true do fetches in background goroutine

[iannucci, 2016/03/01 00:42:41]

So, reading the go memory model, I don't think background goroutines are
actually guaranteed to run. Practically, they do right now, but it worries me
somewhat that the go compiler reserves the right to remove code that doesn't
happen-before anything else.

[Vadim Sh., 2016/03/01 01:11:26]

async mode is not used currently, removed it until it is really needed

 
-        lock           sync.Mutex      // protects the guts below
-        current        *Value          // currently known value or nil if not fetched
-        currentFetcher context.Context // non nil if some goroutine is fetching now
+        lock              sync.Mutex      // protects the guts below
+        current           *Value          // currently known value or nil if not fetched
+        currentFetcherCtx context.Context // non-nil if some goroutine is fetching now
 }
 
-// Peek returns currently cached value if there's one or zero Value{} if not.
-// It doesn't try to fetch a value.
-func (s *Slot) Peek() Value {

[Vadim Sh., 2016/02/29 22:38:28]

it wasn't used outside tests and was racy, so I removed it.

-        if s.current == nil {
-                return Value{}
-        }
-        return *s.current
-}
-
-// Get returns stored value if it is still fresh. It may return slightly stale
-// copy if some other goroutine is fetching a new copy now. If there's no cached
-// copy at all, blocks until it is retrieved (even if slot is configured with
-// Async = true). Returns an error only when Fetcher returns an error.
-func (s *Slot) Get(c context.Context) (Value, error) {
+// Get returns stored value if it is still fresh.
+//
+// It may return slightly stale copy if some other goroutine is fetching a new
+// copy now. If there's no cached copy at all, blocks until it is retrieved
+// (even if the slot is configured with Async = true).
+//
+// Returns an error only when Fetcher returns an error. If no error is returned,
+// Value.Value is guaranteed to be non-nil.
+func (s *Slot) Get(c context.Context) (result Value, err error) {
         now := clock.Now(c)
 
-        // Set in the local function below, used it fetch is needed.
-        var (
-                ctx     context.Context
-                fetchCb Fetcher
-                prevVal Value
-                async   bool
-        )
+        // This lock protects the guts of the slot and makes sure only one goroutine
+        // is doing an initial fetch. It is released explicitly before returns (and
+        // not via defer) to simplify code path that needs to release the lock before
+        // exiting the function.

[iannucci, 2016/03/01 00:42:41]

why not just have a sub-function do this?

rslt, ok := func() (...) {
   Lock()
   defer Unlock()
   // stuff that needs lock
}()

if ok {
  // stuff that happens after-lock if we did the thingy
}


IMO it makes the code easier to maintain later (e.g. you can't accidentally add
an `if err != nil { log; return err}` deadlock)

[Vadim Sh., 2016/03/01 01:11:26]

It used to be like that and I found it hard to read, because this inner function
captured an enormous closure (like 5 variables). Unfortunately, it is
unaviodable for panic-safety, so I resurrected it (changing slightly).

+        s.lock.Lock()
 
-        // If done is true, val and err are returned right away.
-        done, val, err := func() (bool, Value, error) {
-                s.lock.Lock()
-                defer s.lock.Unlock()
-
-                // Still fresh? Return right away.
-                if s.current != nil && now.Before(s.current.Expiration) {
-                        return true, *s.current, nil
-                }
-
-                // Fetching the value for the first time ever? Do it under the lock because
-                // there's nothing to return yet. All goroutines would have to wait for this
-                // initial fetch to complete. They'll all block on s.lock.Lock() above.
-                if s.current == nil {
-                        val, err := s.Fetcher(c, Value{})
-                        if err != nil {
-                                return true, Value{}, err
-                        }
-                        s.current = &val
-                        return true, val, nil
-                }
-
-                // We have a cached copy and it has expired. Maybe some other goroutine is
-                // fetching it already? Returns the stale copy if so.
-                if s.currentFetcher != nil {
-                        return true, *s.current, nil
-                }
-
-                // No one is fetching the value now, we should do it. Release the lock while
-                // fetching to allow other goroutines to grab the stale copy.
-                timeout := 5 * time.Second
-                if s.Timeout != 0 {
-                        timeout = s.Timeout
-                }
-                s.currentFetcher, _ = context.WithTimeout(c, timeout)
-                ctx = s.currentFetcher
-                fetchCb = s.Fetcher
-                prevVal = *s.current
-                async = s.Async
-                return false, Value{}, nil
-        }()
-        if done {
-                return val, err
+        // A cached value exists and it is still fresh? Return it right away.
+        if s.current != nil && now.Before(s.current.Expiration) {
+                result = *s.current
+                s.lock.Unlock()
+                return
         }
 
-        fetch := func() (val Value, err error) {
+        // Fetching the value for the first time ever? Do it under the lock because
+        // there's nothing to return yet. All goroutines would have to wait for this
+        // initial fetch to complete. They'll all block on s.lock.Lock() above.
+        if s.current == nil {
+                result, err = doFetchNoPanic(c, s.Fetcher, Value{})
+                if err == nil {
+                        s.current = &result
+                }
+                s.lock.Unlock()
+                return
+        }
+
+        // We have a cached copy but it has expired. Maybe some other goroutine is
+        // fetching it already? Returns the cached stale copy if so.
+        if s.currentFetcherCtx != nil {
+                result = *s.current
+                s.lock.Unlock()
+                return
+        }
+
+        // No one is fetching the value now, we should do it. Prepare new context
+        // that will be used to do the fetch once lock is released.
+        timeout := 5 * time.Second
+        if s.Timeout != 0 {
+                timeout = s.Timeout
+        }
+        s.currentFetcherCtx, _ = context.WithTimeout(c, timeout)
+
+        // Copy lock-protected guts into local variables before releasing the lock.
+        currentFetcherCtx := s.currentFetcherCtx
+        fetchCb := s.Fetcher
+        prevVal := *s.current
+        async := s.Async
+
+        // Release the lock to allow other goroutines to grab stale copy while fetch
+        // is in the progress.
+        s.lock.Unlock()
+
+        // fetch finishes the fetch and updates cached value.
+        fetch := func() (result Value, err error) {
                 defer func() {
                         s.lock.Lock()
                         defer s.lock.Unlock()
-                        s.currentFetcher = nil
-                        if err == nil && val.Value != nil {
-                                s.current = &val
+                        s.currentFetcherCtx = nil
+                        if err == nil {
+                                s.current = &result
                         }
                 }()
-                return fetchCb(ctx, prevVal)
+                return doFetchNoPanic(currentFetcherCtx, fetchCb, prevVal)
         }
 
         if async {
+                // Start async fetch and return stale copy while it is running.
                 go fetch()
                 return prevVal, nil
         }
         return fetch()
 }
+
+// doFetchNoPanic calls fetcher callback, trapping panics and validating
+// return value.

[iannucci, 2016/03/01 00:42:41]

let it burrrnnnnnnnnnn....

                 _ ._  _ , _ ._
               (_ ' ( `  )_  .__)
             ( (  (    )   `)  ) _)
            (__ (_   (_ . _) _) ,__)
                `~~`\ ' . /`~~`
                ,::: ;   ; :::,
               ':::::::::::::::'
          __________/_ __ \____________

[Vadim Sh., 2016/03/01 01:11:26]

Done.

+func doFetchNoPanic(ctx context.Context, cb Fetcher, prev Value) (result Value, err error) {
+        defer func() {
+                if r := recover(); r != nil {
+                        err = fmt.Errorf("panic caught in lazyslot.Slot Fetcher - %s", r)
+                        return
+                }
+                switch {
+                case err == nil && result.Value == nil:
+                        err = fmt.Errorf("lazyslot.Slot Fetcher returned nil value")
+                case err != nil:
+                        result = Value{}
+                }
+        }()
+        return cb(ctx, prev)
+}