*: Use http.Request.WithContext instead of Cancel #7269
Conversation
|
CI failure is not related: see #6934 |
lease/leasehttp/http.go
Outdated
| @@ -239,7 +239,6 @@ func TimeToLiveHTTP(ctx context.Context, id lease.LeaseID, keys bool, url string | |||
| return nil, derr | |||
| } | |||
| case <-ctx.Done(): | |||
There was a problem hiding this comment.
do we need to wait on this anymore? if ctx is cancelled, the cc.Do at line 212 will return, thus trigger line 237. Right?
There was a problem hiding this comment.
we can remove the go routine at 211 with this change i think.
There was a problem hiding this comment.
What you are saying is right will change it.
| if ok { | ||
| closeCh := closeNotifier.CloseNotify() | ||
| go func() { | ||
| select { | ||
| case <-closeCh: | ||
| atomic.StoreInt32(&requestClosed, 1) | ||
| plog.Printf("client %v closed request prematurely", clientreq.RemoteAddr) | ||
| cancel() |
There was a problem hiding this comment.
we should still keep this cancel. the go routine exit at line 117 will not trigger the defer func at line 114.
There was a problem hiding this comment.
@xiang90 its not required there. It will still work fine. The reason being the code is written in such a way that on either of these select triggered, the method will exit and also cancel will get called. The defer func after the select causes it to end i think.
There was a problem hiding this comment.
i read the code again. what you said makes sense. Thanks.
|
LGTM. defer to @heyitsanthony |
|
lgtm |
#6953