common/lazyslot: Simplify code a little bit.

common/lazyslot/lazyslot.go

Index: common/lazyslot/lazyslot.go
diff --git a/common/lazyslot/lazyslot.go b/common/lazyslot/lazyslot.go
index a7ae392cf670f4c73ea279a20a1907562697ac67..dd21a84a5d96dd6715cfbc9bae77312cfaaeb6ce 100644
--- a/common/lazyslot/lazyslot.go
+++ b/common/lazyslot/lazyslot.go
@@ -3,12 +3,15 @@
 // 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"
 
@@ -25,106 +28,125 @@ type Value struct {
 	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
-	)
-
-	// 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
-		}
+	// 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()
+
+	// 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
+	}
 
-		// 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
+	// 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 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
-		}
+	// 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. 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
+	// 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 := func() (val Value, err error) {
+	// 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)
+}