[CDEC-356] Force ntp check #3323
Conversation
networking/src/Ntp/Client.hs
Outdated
| @@ -192,6 +204,13 @@ sendLoop cli addrs = do | |||
| retry | |||
| logDebug "collected all responses" | |||
|
|
|||
| forceRequest = | |||
There was a problem hiding this comment.
I'm confused about the name. It looks like this doesn't really force anything, rather it waits for something.
|
@avieth this is required by the wallet. There are occasions where a wallet user might be asked to rerun the ntp check. A user will click a button and the wallet backend will receive a request to force ntp check. This is a minimal change to support this functionality. The wallet can re-use the already exposed |
networking/src/Ntp/Client.hs
Outdated
| case cmd of | ||
| Nothing -> cancel a | ||
| Just SendRequest | ||
| -> sendLoop cli addrs |
There was a problem hiding this comment.
Is it right to recurse here? It's not tail recursive. Is it possible that we get a stack overflow?
There was a problem hiding this comment.
I don't think that's a real danger, it can only be triggered by the end user action. But maybe there is a simple solution to get tail recursion here, I'll look into that.
There was a problem hiding this comment.
If we recurse here, is it still necessary to do lines 194-196? If not, put them in the Nothing case.
There was a problem hiding this comment.
I think you're right, we don't need to recurs here at all. It's fine if we just return () and recurs only in the tail position. The timeout poll (wait a) will wait until either the poll timeout or somebody (re)set the ncStatus :: TVar NtpStatus.
networking/src/Ntp/Client.hs
Outdated
| -- after @'updateStatus'@ @'ntpStatus'@ is guaranteed to be | ||
| -- different from @'NtpSyncPending'@, now we can wait until it was | ||
| -- changed back to @'NtpSyncPending'@ to force a request. | ||
| waitForRequest |
There was a problem hiding this comment.
One more question, this time about the withAsync concurrency here. Why is it needed? Could this not be done sequentially by sendPacketing, then timeout respTimeout waitForResponses, then waitForRequest?
There was a problem hiding this comment.
That's true, unless the ntp servers answer faster than haskell executes waitForResponses ;)
|
@avieth I was looking in the
This means we can further simplify the ntp client, there's no need to distinguish ipv4 and ipv6 sockets. |
Nice. No mention of cross-platform support though. Would be good to verify that. |
Description
Force ntp check by overwriting
NtpStatusmutable cell.Linked issue
https://iohk.myjetbrains.com/youtrack/issue/CDEC-356
Type of change
Developer checklist
Testing checklist
QA Steps
Screenshots (if available)