polygon/sync: blockdownloader to use common.Sleep #10467
Conversation
polygon/sync/block_downloader.go
Outdated
| @@ -149,7 +149,7 @@ func (d *blockDownloader) downloadBlocksUsingWaypoints(ctx context.Context, wayp | |||
| "sleepSeconds", d.notEnoughPeersBackOffDuration.Seconds(), | |||
| ) | |||
|
|
|||
| time.Sleep(d.notEnoughPeersBackOffDuration) | |||
| common.Sleep(ctx, d.notEnoughPeersBackOffDuration) | |||
There was a problem hiding this comment.
are you sure that don't need to check err?
There was a problem hiding this comment.
common.Sleep does not return an err
There was a problem hiding this comment.
seems like you and @mh0lt may be suggesting that it should be returning an error if the parent ctx timeout/got cancelled so the caller knows - is this right?
There was a problem hiding this comment.
ok talked on chat with Mark so will make changes to:
- return err from common.Sleep coming from parentCtx.Err (once it is done)
- use time.NewTimer for the sleep time since it plays better with parentCtx.Done (timer gives us a channel to block on like ctx.Done)
There was a problem hiding this comment.
I have the same question as alex.
I assume the benefit of this over time.Sleep is that if the context gets cancelled then the sleep will return, however the way this is used here this is ignored. I think it should possibly check the err and return if cancelled rather than timed out ?
Although looking at the code - it seems like it forces a cancel - rather than using the parent - so perhaps this is a moot point ?
|
@mh0lt @AskAlexSharov ---> talked offline, the suggestion from Mark is to return an err to the caller to indicate to that the ctx has been cancelled/timed out |
Original intent of this PR was to replace 1 usage of
time.Sleepinpolygon/sync/block_downloader.gowithcommon.Sleepwhich is context-aware.However when doing this we discussed that common.Sleep can be improved to return an
errto the caller when the parent ctx has been cancelled so that the caller is made aware of that without having to make subsequent checks.