No wonder I didn't find related questions, the method names in the two packages are different: One is Forget and the other is ForgetUnshared
在2022年9月23日星期五 UTC+8 11:05:51<cuong.m...@gmail.com> 写道： > Seems to me this commit is not port to the internal singleflight: > https://github.com/golang/sync/commit/56d357773e8497dfd526f0727e187720d1093757 > > On Friday, September 23, 2022 at 9:23:29 AM UTC+7 atomic wrote: > >> Thank you so much, so happy, you are amazing. >> You answered a question that has been bothering me for days, I opened an >> issue on github, can you submit a pr to fix this? >> https://github.com/golang/go/issues/55343 >> >> 在2022年9月23日星期五 UTC+8 06:23:13<Brian Candler> 写道： >> >>> And here's a proof-of-concept fix which seems to do the job: >>> >>> --- main.go.orig 2022-09-21 13:14:10.000000000 +0100 >>> +++ main.go 2022-09-22 23:19:54.000000000 +0100 >>> @@ -27,6 +27,7 @@ >>> // not written after the WaitGroup is done. >>> dups int >>> chans []chan<- Result >>> + forgotten bool >>> } >>> >>> // Group represents a class of work and forms a namespace in >>> @@ -101,7 +102,9 @@ >>> c.wg.Done() >>> >>> g.mu.Lock() >>> - delete(g.m, key) >>> + if !c.forgotten { >>> + delete(g.m, key) >>> + } >>> for _, ch := range c.chans { >>> ch <- Result{c.val, c.err, c.dups > 0} >>> } >>> @@ -121,6 +124,7 @@ >>> return true >>> } >>> if c.dups == 0 { >>> + c.forgotten = true >>> delete(g.m, key) >>> return true >>> } >>> >>> On Thursday, 22 September 2022 at 23:16:22 UTC+1 Brian Candler wrote: >>> >>>> OK, I think I have it. It's ugly. >>>> >>>> Firstly, note that multiple instances of doCall can be running for the >>>> same key. This happens when: >>>> >>>> 1. you invoke DoChan. This inserts a 'c' (call struct) into the map >>>> and starts doCall in a goroutine. >>>> 2. at this point it's not shared: i.e. you don't call DoChan again with >>>> the same key (yet). >>>> 3. you invoke ForgetUnshared on this key. This "detaches" it, but >>>> doCall carries on running. It has its own local copy of 'c' so it knows >>>> where to send the result, even though the map is now empty. >>>> 4. you invoke DoChan again with the same key. This inserts a new 'c' >>>> into the map and starts a new doCall goroutine. >>>> >>>> At this point, you have two instances of doCall running, and the map is >>>> pointing at the second one. >>>> >>>> This is where it gets ugly. >>>> >>>> 5. you invoke DoChan yet again with the same key. This turns it into a >>>> shared task, with c.dups > 0, len(c.chans) > 1. >>>> 6. the first instance of doCall terminates. At this point it >>>> unconditionally removes the key from the map - even though it had >>>> previously been removed by ForgetUnshared! >>>> >>>> func (g *Group) doCall(c *call, key string, fn func() (interface{}, >>>> error)) { >>>> c.val, c.err = fn() >>>> c.wg.Done() >>>> >>>> g.mu.Lock() >>>> * delete(g.m, key) // <<<< NOTE* >>>> for _, ch := range c.chans { >>>> ch <- Result{c.val, c.err, c.dups > 0} >>>> } >>>> g.mu.Unlock() >>>> } >>>> >>>> So, even though it's the first instance of doCall which is terminating, >>>> it's removing the second instance of doCall from the map. This is now >>>> also >>>> a detached task. >>>> >>>> 7. In one of the two goroutines, the timeout event occurs. It calls >>>> ForgetUnshared, which happily returns true because the key does not exist >>>> in the map - and therefore you proceed to cancel the context. >>>> >>>> But actually a task with this key *is* running; and furthermore, it is >>>> a shared task, with 2 channel receivers. >>>> >>>> 8. Once the sleep has completed in the task function, it notices that >>>> the context is cancelled and returns an error. >>>> >>>> 9. doCall sends the resulting error down multiple channels (those you >>>> started in steps 4 and 5 above) >>>> >>>> 10. The select { case res := <-ch } triggers in the *other* goroutine - >>>> the one which didn't have a timeout. Hence it receives the error, and >>>> that's where you panic(). >>>> >>>> On Thursday, 22 September 2022 at 20:37:07 UTC+1 Brian Candler wrote: >>>> >>>>> OK, I see where you're coming from - and I agree, this is a difficult >>>>> one! >>>>> >>>>> The point you were making is that >>>>> >>>>> if g.ForgetUnshared(key) { >>>>> cancel() >>>>> } >>>>> >>>>> should only invoke cancel() if this result wasn't shared: i.e. there's >>>>> only one receiver in the c.chans array, and c.dups == 0. So where's the >>>>> race, given that everything in g is done under a mutex? >>>>> >>>>> What I have discovered so far is: when g.ForgetUnshared(key) returns >>>>> true and the problem occurs, the key is not present in the map (as >>>>> opposed >>>>> to being present with c.dups == 0). But I've not been able to work out >>>>> why >>>>> yet. >>>>> >>>>> Incidentally, a minor style observation: you passed in ctx to your go >>>>> func(...), but not cancel. As far as I can see, both ctx and cancel are >>>>> local variables which drop immediately out of scope - there's no way they >>>>> can be modified later outside of the goroutine. So I believe you don't >>>>> need to pass ctx at all: you can access it via the closure. But if you >>>>> do >>>>> pass one "to be on the safe side", then I think the other should be >>>>> passed >>>>> as well - otherwise it's confusing why you passed in only one. >>>>> >>>>> In fact, in this case, you could move the ctx/cancel creation inside >>>>> the go func(...) anyway. The only thing which needs to be outside is >>>>> the wg.Add(1). >>>>> >>>>> On Thursday, 22 September 2022 at 03:12:47 UTC+1 atomic wrote: >>>>> >>>>>> > Also notice that the random time you pick for cancelTime can be >>>>>> longer than the different random time you sleep inside the goroutine >>>>>> (i.e. >>>>>> the function which you pass to DoChan). Hence the goroutine can return >>>>>> a >>>>>> result, before the cancelTime is reached. >>>>>> >>>>>> Although the goroutine can return a result before cancelTime arrives, >>>>>> the returned result should not be err because I haven't had time to call >>>>>> cancel(). >>>>>> 在2022年9月21日星期三 UTC+8 20:18:30<Brian Candler> 写道： >>>>>> >>>>>>> Notice that DoChan starts a goroutine for the task... >>>>>>> >>>>>>> go g.doCall(c, key, fn) >>>>>>> >>>>>>> ... and then returns immediately. >>>>>>> >>>>>>> Also notice that the random time you pick for cancelTime can be >>>>>>> longer than the different random time you sleep inside the goroutine >>>>>>> (i.e. >>>>>>> the function which you pass to DoChan). Hence the goroutine can return >>>>>>> a >>>>>>> result, before the cancelTime is reached. >>>>>>> >>>>>>> Try this modification: >>>>>>> >>>>>>> --- main.go.orig 2022-09-21 13:14:10.000000000 +0100 >>>>>>> +++ main.go 2022-09-21 13:13:43.000000000 +0100 >>>>>>> @@ -144,7 +144,7 @@ >>>>>>> defer wg.Done() >>>>>>> >>>>>>> ch, _ := g.DoChan(key, func() (interface{}, error) { >>>>>>> - time.Sleep(randTimeout()) >>>>>>> + time.Sleep(5000 * time.Millisecond) >>>>>>> if ctx.Err() == context.Canceled { >>>>>>> return nil, fmt.Errorf("callUUID=[%d] >>>>>>> err=[%s]", uuid, ctx.Err()) >>>>>>> } >>>>>>> @@ -152,7 +152,7 @@ >>>>>>> }) >>>>>>> >>>>>>> // randomly choose a timeout to cancel >>>>>>> - cancelTime := time.After(randTimeout()) >>>>>>> + cancelTime := time.After(10 * time.Millisecond) >>>>>>> select { >>>>>>> case <-cancelTime: >>>>>>> // cancel only if no other goroutines share >>>>>>> >>>>>>> On Wednesday, 21 September 2022 at 10:01:22 UTC+1 atomic wrote: >>>>>>> >>>>>>>> Thanks for your reply, but I still don't understand why time.Sleep >>>>>>>> is causing my test program to panic. >>>>>>>> >>>>>>>> In fact, this is a real online environment problem. My application >>>>>>>> uses http.Client.Do(), but it occasionally has errors: [lookup >>>>>>>> xxxxx on xxxxx: dial udp xxxxx: operation was canceled], after looking >>>>>>>> at >>>>>>>> the code, I found that it may be There is a problem with >>>>>>>> ForgetUnshared, >>>>>>>> lookupIPAddr uses ForgetUnshared: >>>>>>>> https://github.com/golang/go/blob/4a4127bccc826ebb6079af3252bc6bfeaec187c4/src/net/lookup.go#L336 >>>>>>>> >>>>>>>> 在2022年9月21日星期三 UTC+8 16:17:35<cuong.m...@gmail.com> 写道： >>>>>>>> >>>>>>>>> Hello, >>>>>>>>> >>>>>>>>> You use time.Sleep in your program, so the behavior is not >>>>>>>>> predictable. In fact, I get it success or panic randomly. >>>>>>>>> >>>>>>>>> You can see https://go-review.googlesource.com/c/sync/+/424114 to >>>>>>>>> see a predictable test of ForgetUnshared . >>>>>>>>> >>>>>>>>> On Wednesday, September 21, 2022 at 1:45:24 PM UTC+7 atomic wrote: >>>>>>>>> >>>>>>>>>> hello >>>>>>>>>> >>>>>>>>>> I find that the `src/internal/singleflight/singleflight.go >>>>>>>>>> ForgetUnshared()` method returns results that are not always expected >>>>>>>>>> >>>>>>>>>> For this I wrote a test code, I copied the code in the >>>>>>>>>> src/internal/singleflight/singleflight.go file to the main package, >>>>>>>>>> and >>>>>>>>>> wrote a main function to test it, if ForgetUnshared() returns >>>>>>>>>> correctly, >>>>>>>>>> this code It should not panic, but the fact that it will panic every >>>>>>>>>> time >>>>>>>>>> it runs, is there something wrong with my understanding of >>>>>>>>>> ForgetUnshared()? >>>>>>>>>> >>>>>>>>>> The test code cannot be run in goplay, so I posted a link: >>>>>>>>>> https://gist.github.com/dchaofei/e07547bce17d94c3e05b1b2a7230f62f >>>>>>>>>> >>>>>>>>>> The go version I use for testing is 1.16, 1.19.1 >>>>>>>>>> >>>>>>>>>> result: >>>>>>>>>> ``` >>>>>>>>>> $ go run cmd/main.go >>>>>>>>>> panic: callUUID=[9314284969 <(931)%20428-4969>] err=[context >>>>>>>>>> canceled] currentUUId=[6980556786] >>>>>>>>>> ``` >>>>>>>>>> >>>>>>>>> -- You received this message because you are subscribed to the Google Groups "golang-nuts" group. To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts+unsubscr...@googlegroups.com. To view this discussion on the web visit https://groups.google.com/d/msgid/golang-nuts/19e72271-d7aa-4e6b-9705-b3689e3974d9n%40googlegroups.com.
