Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1750023002:
common/lazyslot: Simplify code a little bit. (Closed)
|
Created:
4 years, 9 months ago by Vadim Sh. Modified:
4 years, 9 months ago CC:
andrew.wang, chromium-reviews, infra-reviews+luci-go_chromium.org, M-A Ruel, tandrii+luci-go_chromium.org, todd Base URL:
https://chromium.googlesource.com/external/github.com/luci/luci-go@master Target Ref:
refs/heads/master Project:
luci-go Visibility:
Public. |
Descriptioncommon/lazyslot: Simplify code a little bit.
Be more strict about nil return value.
BUG=590594
R=dnj@chromium.org, iannucci@chromium.org
Committed: https://github.com/luci/luci-go/commit/e3dd046f72f64f6213a504101b26f345834a903f
Patch Set 1 #
Total comments: 10
Patch Set 2 : don't trap panics #
Dependent Patchsets: Messages
Total messages: 14 (4 generated)
Please read lazyslot.go end-to-end (it is only 150 lines with extensive comments). I looked very carefully at the code and couldn't find any races in Get. I've restructured it a little bit (replacing 'defer' unlock with explicit ones, IMHO it makes code simpler in this case). No logic changes. Also now any panics in Fetch are caught and converted to errors, just in case... Also validating that Value.Value is non-nil now. Hopefully it will help us to figure out where nil sneaks in. https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go File common/lazyslot/lazyslot.go (left): https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:46: func (s *Slot) Peek() Value { it wasn't used outside tests and was racy, so I removed it.
vadimsh@chromium.org changed reviewers: + iannucci@chromium.org
Robbie, can you take a look too?
IMO, we really really really shouldn't be eating panics unless it's explicitly the purpose of the library (e.g. PanicCatcher to log panics as Errors in appengine). See the nested function I approached to make the code safer and not require the panic-swallowing function. https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go File common/lazyslot/lazyslot.go (right): https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:36: // Panics inside Fetcher will be caught and converted to errors. 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. https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:46: Async bool // if true do fetches in background goroutine 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. https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:67: // exiting the function. 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) https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:137: // return value. let it burrrnnnnnnnnnn.... _ ._ _ , _ ._ (_ ' ( ` )_ .__) ( ( ( ) `) ) _) (__ (_ (_ . _) _) ,__) `~~`\ ' . /`~~` ,::: ; ; :::, ':::::::::::::::' __________/_ __ \____________
Description was changed from ========== common/lazyslot: Catch panics, simplify code a little bit. BUG=590594 R=dnj@chromium.org ========== to ========== common/lazyslot: Simplify code a little bit. Be more strict about nil return value. BUG=590594 R=dnj@chromium.org, iannucci@chromium.org ==========
https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go File common/lazyslot/lazyslot.go (right): https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:36: // Panics inside Fetcher will be caught and converted to errors. On 2016/03/01 00:42:41, iannucci wrote: > 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. Okay. It used to work like that. Reverted. https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:46: Async bool // if true do fetches in background goroutine On 2016/03/01 00:42:41, iannucci wrote: > 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. async mode is not used currently, removed it until it is really needed https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:67: // exiting the function. On 2016/03/01 00:42:41, iannucci wrote: > 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) 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). https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:137: // return value. On 2016/03/01 00:42:41, iannucci wrote: > let it burrrnnnnnnnnnn.... > > _ ._ _ , _ ._ > (_ ' ( ` )_ .__) > ( ( ( ) `) ) _) > (__ (_ (_ . _) _) ,__) > `~~`\ ' . /`~~` > ,::: ; ; :::, > ':::::::::::::::' > __________/_ __ \____________ Done.
This seems similar to common/promise. Is there any interest in consolidating? https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go File common/lazyslot/lazyslot.go (right): https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... common/lazyslot/lazyslot.go:36: // Panics inside Fetcher will be caught and converted to errors. On 2016/03/01 00:42:41, iannucci wrote: > 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. +1
On 2016/03/01 01:21:45, dnj wrote: > This seems similar to common/promise. Is there any interest in consolidating? > The core feature of lazyslot is that its value expires after some timeout (and then some gorouting, and _only one_, refetches it). I think adding expiration to Promise will unnecessarily complicate it, since it's rare use case for Promise. Promise can be potentially used as implementation detail of lazyslot.Slot, but I'm not sure it will shorten the code very much (it is already quite short), but will add unnecessary dependency. > https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go > File common/lazyslot/lazyslot.go (right): > > https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... > common/lazyslot/lazyslot.go:36: // Panics inside Fetcher will be caught and > converted to errors. > On 2016/03/01 00:42:41, iannucci wrote: > > 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. > > +1 Ack. It doesn't trap panics any more.
On 2016/03/01 01:31:28, Vadim Sh. wrote: > On 2016/03/01 01:21:45, dnj wrote: > > This seems similar to common/promise. Is there any interest in consolidating? > > > The core feature of lazyslot is that its value expires after some timeout (and > then some gorouting, and _only one_, refetches it). I think adding expiration to > Promise will unnecessarily complicate it, since it's rare use case for Promise. > Err.. wrong focus here :) Only one goroutine fetches, while all other happily use stale data. In GAE case, only one request goroutine will pay the penalty of refetching settings from the datastore, while all others will harmlessly use a slightly stale copy while fetch is on-going. tl;dr I think it's sufficiently different use case from simple Promise and merging them will increase overall complexity. > Promise can be potentially used as implementation detail of lazyslot.Slot, but > I'm not sure it will shorten the code very much (it is already quite short), but > will add unnecessary dependency. > > > https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go > > File common/lazyslot/lazyslot.go (right): > > > > > https://codereview.chromium.org/1750023002/diff/1/common/lazyslot/lazyslot.go... > > common/lazyslot/lazyslot.go:36: // Panics inside Fetcher will be caught and > > converted to errors. > > On 2016/03/01 00:42:41, iannucci wrote: > > > 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. > > > > +1 > > Ack. It doesn't trap panics any more.
lgtm
The CQ bit was checked by vadimsh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1750023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1750023002/20001
Message was sent while issue was closed.
Description was changed from ========== common/lazyslot: Simplify code a little bit. Be more strict about nil return value. BUG=590594 R=dnj@chromium.org, iannucci@chromium.org ========== to ========== common/lazyslot: Simplify code a little bit. Be more strict about nil return value. BUG=590594 R=dnj@chromium.org, iannucci@chromium.org Committed: https://github.com/luci/luci-go/commit/e3dd046f72f64f6213a504101b26f345834a903f ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://github.com/luci/luci-go/commit/e3dd046f72f64f6213a504101b26f345834a903f |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
