[CDEC-439] Cleaning ntp client #3264

coot · 2018-07-17T18:51:02Z

Description

I cleaned the ntp client a bit.

The ntp client resolves addresses and takes first ipv4 and ipv6 addresses. It now will also try the other address in case of failure (it may happen that ipv4/6 is not supported by the os).

And I added tests:

serialization / deserialization of ntp packets
computation of clock offset
the coercions between ntp time and unix time

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CDEC-439

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

erikd · 2018-07-18T01:37:37Z

networking/src/Ntp/Client.hs

-
+    let poll        = ntpPollDelay (ncSettings cli)
+
+    _ <- withAsync


That looks like you are discarding the result of a computation. Since the last statement in the computation is cancel a whose result type is IO (), the above could be written as:

() <- withAsync

which is more obviously correct and hence easier for the reviewer.

That's true. PureScript has a class for types which can be ignored without a warning (with an instance for ()), it would be nice to have it in Haskell.

erikd · 2018-07-18T01:56:38Z

networking/src/Ntp/Client.hs

+    ncStatus <- newTVarIO NtpSyncPending
+    -- using async so the ntp thread will be left running even if the parent
+    -- thread finished.
+    _ <- liftIO $ async (spawnNtpClient ntpSettings ncStatus)


How many times is this function likely to be called? Does spawnNtpClient ever terminate?

If it never terminates, how do we make sure we don't get hundreds, thousands or even more threads?

If if just runs, does it job and then terminates, it would be nice to have a comment to that effect.

It just runs once, a comment should be in place indeed.

erikd · 2018-07-18T02:36:44Z

networking/src/Ntp/Client.hs

+            (<>) <$> (Option <$> createAndBindSock IPv4 addrs)
+                 <*> (Option <$> createAndBindSock IPv6 addrs)
+
+    handlerE :: SomeException -> IO (Option Sockets)


We're using Control.Exception.Safe in this module so SomeException does not include async exxecptions like stack and heap overflow, right?

Yes, it's a bit inconsistent though: Ntp.Client is using safe-exceptions while Ntp.Util is using Contorl.Exception (but it's never catching async exceptions, only IOException and one custom exception). I think we should just catch IOExceptions in Ntp.Client as well rather than SomeException.

coot · 2018-07-18T12:10:06Z

@erikd I updated with your comments with two additions:

now whole Ntp is using Control.Exception catching IOException, instead of mixing safe-exceptions with Control.Exception.
I moved NtpConfiguration from infra to networking

Plus: remove safe-exception, and instead catch only `IOException`s.

parsonsmatt · 2018-07-20T17:58:14Z

networking/src/Ntp/Util.hs

+    = EBFirst  !a
+    | EBSecond !b
+    | EBBoth   !a !b
+    deriving (Show, Eq, Ord)


aka the these datatype, though this is strict

Indeed, the strictness doesn't matter that much, since we are just holding max three values, so if you prefer we can pull these as a dependency. these hasn't been yet used anywhere in cardano-sl but it's a small dependency so I don't mind.

parsonsmatt · 2018-07-20T17:59:32Z

networking/test/Test/NtpSpec.hs

+{-# LANGUAGE TypeApplications #-}
+
+module Test.NtpSpec
+    ( spec


parsonsmatt · 2018-07-20T18:01:04Z

networking/test/Test/NtpSpec.hs

+            let offset = clockOffsetPure npoNtpPacket npoDestinationTime
+            in npoOffset === offset
+    describe "realMcsToNtp" $ do
+        it "should be right inverse of ntpToRealMcs" $ property $ \(NtpMicrosecond x) ->


hspec has a shorthand for this; it msg (property f) can be prop msg f

parsonsmatt · 2018-07-20T18:02:07Z

networking/test/Test/NtpSpec.hs

+            x === uncurry ntpToRealMcs (realMcsToNtp x)
+        it "should be left inverse of ntpToRealMcs" $ property $ \(NtpTime x@(sec, frac)) ->
+            let (sec', frac') = realMcsToNtp (uncurry ntpToRealMcs x)
+            in sec === sec' .&&. frac `div` 4310 === frac' `div` 4310


4310 is a bit of a magic number here. What's the significance? Could it be named?

It is 1000000 div 232, in the ntp spec time is represented in seconds + a fraction of a second which resolved to 232 picoseconds, so there are exactly 4310 of them in a milisecond.

avieth

Will give it another pass later. It's a lot to take in!

avieth · 2018-07-20T19:13:44Z

networking/src/Ntp/Client.hs

@@ -66,7 +75,7 @@ data NtpClientSettings = NtpClientSettings
 data NtpClient = NtpClient
    { ncSockets  :: TVar Sockets
      -- ^ Ntp client sockets: ipv4 / ipv6 / both.
-    , ncState    :: TVar (Maybe [NtpOffset])
+    , ncState    :: TVar [NtpOffset]


Was there previously a semantic distinction between Nothing and Just []?

There was, but it wasn't necessary. It was used to not accept request which came late. Every packet comes with information when it was sent so we can discard it based on that information. This is now done in clockOffset function

avieth · 2018-07-20T19:18:24Z

networking/src/Ntp/Client.hs

+        (do
+            -- wait for responses and update status
+            _ <- timeout respTimeout waitForResponses
+            updateStatus cli


Could this part be done purely? Instead of having waitForResponses mutate some TVar, have it return the responses directly, then make updateStatus take it as an argument?

Good point, I was actually thinking about doing this too. I haven't done it because we need ncState :: NtpClient -> TVar [NtpOffset] variable to communicate between the listener thread and sending thread. But we can change how the client is run: if we start the listener thread in this place we will not need this tvar. The communication will be done just using ncStatus.

We need to listen on two sockets, so I think there's no way around impure ncState. Maybe you have some other idea?

I wonder if having a receiving thread (startReceive) is the wrong way around. Do we ever expect to get a message without first sending one? That's to say, are we acting as an ntp server as well as a client? If not, maybe we should recvFrom the socket on the same thread after sending on it. That would give a simpler implementation, with less concurrency and fewer mutable cells.

It is, but I haven't changed that. I just didn't want to rewrite everything. I will add a todo, and let's move on.

avieth · 2018-07-20T19:22:10Z

networking/src/Ntp/Client.hs

+    handleIOException addressFamily e = do
+        logDebug $ sformat ("startReceive failed with reason: "%shown) e
+        threadDelay 5000000
+        udpLocalAddresses >>= createAndBindSock addressFamily >>= \case


Could we somehow reuse/consolidate mkSockets from below?

Yeah, good idea.

Giving this a second thought: in this place we are just recreating the socket that failed, while mkSockets creates both of them, i.e. IPv4 and IPv6; so I don't think we should reuse it here.

avieth · 2018-07-20T19:23:30Z

networking/src/Ntp/Packet.hs


 instance Binary NtpPacket where
    put NtpPacket{..} = do
        putWord8 ntpParams
+        putWord8 0
+        putInt8 ntpPoll
+        putWord8 0


A change to the binary format? Is this some standardised NTP format?

Not really, I added bits just so we have a test that put and get are inverse of each other.

The added quickcheck test

They weren't inverse prior to this change?
So I'm guessing the replicateM_ 4 $ putWord8 0 was there beecause ntpPoll was assumed to be 0. Here there's a putInt8 rather than putWord8. Same encoding?
Then there's the replicateM_ 9 $ PutWord32be 0, which is replaced by replicateM_ 5 $ putWord32be 0 but where are the remaining 4? ntpOriginTime ^.. each and ntpReceivedTime ^.. each give 2 Word32s each? Lens combinators are annoying hard to search for.

* use `These` from `these` package * `prop` in tests * comment why `4310` appears in the test

avieth

Some final requests. If you know that everything is still working then LGTM.

avieth · 2018-07-27T16:38:41Z

networking/src/Ntp/Packet.hs


 instance Binary NtpPacket where
    put NtpPacket{..} = do
        putWord8 ntpParams
+        putWord8 0
+        putInt8 ntpPoll
+        putWord8 0


They weren't inverse prior to this change?
So I'm guessing the replicateM_ 4 $ putWord8 0 was there beecause ntpPoll was assumed to be 0. Here there's a putInt8 rather than putWord8. Same encoding?
Then there's the replicateM_ 9 $ PutWord32be 0, which is replaced by replicateM_ 5 $ putWord32be 0 but where are the remaining 4? ntpOriginTime ^.. each and ntpReceivedTime ^.. each give 2 Word32s each? Lens combinators are annoying hard to search for.

avieth · 2018-07-27T16:49:02Z

networking/src/Ntp/Packet.hs

+    let -- microseconds
+        secMicro :: Integer
+        secMicro = (fromIntegral sec - ntpTimestampDelta) * 1000000
+        -- Each fraction resolves to 232 pico seconds (`232 ≈ 10000000/4294`)


There's an extra 0 in the numerator.

The explanation could be better. We divide 1 second into 2 ^ 32 parts, giving 2.3283064365386963e-10 as the quantum. A picosecond is 10e-12 of a second, so this is 232 picoseconds.

It wasn't. Though previously Int was used as type for poll in NtpPacket we put 0 :: Word8 when encoding it (because it does not matter). I changed Int to Int8, the spec sais that:

Some time values are represented in exponent format, including the
precision, time constant, and poll interval. These are in 8-bit
signed integer format in log2 (log base 2) seconds.

So I Int8 should be used. Before it was Int. We ignore this field, but let's follow the spec.

But put and get weren't inverses because when we were encoding a packet we always put 0 in the origin and received time. This also does not matter for us, since we're just a client, but why not follow the spec if it does not cost much.

Indeed each the result of realMcsToNtp return a pair of Word32 (seconds since 1 Jan 1900 and fractions).

Sorry, I put this comment under wrong thread.

[CDEC-439] Cleaning ntp client

…hk/coot/cdec-439 [CDEC-439] Cleaning ntp client

coot requested review from avieth and dcoutts as code owners July 17, 2018 18:51

erikd reviewed Jul 18, 2018

View reviewed changes

coot requested review from adinapoli-iohk, akegalj, denisshevchenko and parsonsmatt as code owners July 18, 2018 10:08

coot force-pushed the coot/cdec-439 branch from f295d8e to 0622260 Compare July 18, 2018 10:28

coot added 3 commits July 19, 2018 12:15

[CDEC-439] ntp refactoring & quickcheck tests

fac6940

[CDEC-439] try another address

2430d47

[CDEC-439] Erik's review

570fa8d

Plus: remove safe-exception, and instead catch only `IOException`s.

coot force-pushed the coot/cdec-439 branch 2 times, most recently from 5bb9194 to 8e17a6f Compare July 19, 2018 12:03

coot added 2 commits July 20, 2018 17:52

[CDEC-439] Move NtpConfiguration to Ntp.Client

0a3ce00

[CDEC-439] updated & spellchecked comments

54fd65d

coot force-pushed the coot/cdec-439 branch from a7b4d71 to 6ef9f79 Compare July 20, 2018 15:53

parsonsmatt approved these changes Jul 20, 2018

View reviewed changes

avieth reviewed Jul 20, 2018

View reviewed changes

coot added 2 commits July 27, 2018 10:13

[CDEC-439] Matt's review

6ac5566

* use `These` from `these` package * `prop` in tests * comment why `4310` appears in the test

[CDEC-439] stylish haskell and pkgs/default.nix

1793ff4

coot force-pushed the coot/cdec-439 branch from 8736747 to 1793ff4 Compare July 27, 2018 08:13

avieth approved these changes Jul 27, 2018

View reviewed changes

[CDEC-439] Alex's review

929fbbb

coot merged commit b4e6c5d into develop Jul 27, 2018

coot deleted the coot/cdec-439 branch July 30, 2018 08:51

KtorZ pushed a commit that referenced this pull request Nov 9, 2018

Merge pull request #3264 from input-output-hk/coot/cdec-439

b98816d

[CDEC-439] Cleaning ntp client

KtorZ pushed a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018

Merge pull request input-output-hk/cardano-sl#3264 from input-output-…

752ad17

…hk/coot/cdec-439 [CDEC-439] Cleaning ntp client

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CDEC-439] Cleaning ntp client #3264

[CDEC-439] Cleaning ntp client #3264

coot commented Jul 17, 2018

erikd Jul 18, 2018

coot Jul 18, 2018

erikd Jul 18, 2018 •

edited

Loading

coot Jul 18, 2018

erikd Jul 18, 2018

coot Jul 18, 2018

coot commented Jul 18, 2018

parsonsmatt Jul 20, 2018

coot Jul 20, 2018

parsonsmatt Jul 20, 2018

parsonsmatt Jul 20, 2018

parsonsmatt Jul 20, 2018

coot Jul 20, 2018

avieth left a comment

avieth Jul 20, 2018

coot Jul 25, 2018

avieth Jul 20, 2018

coot Jul 25, 2018

coot Jul 25, 2018

avieth Jul 25, 2018 •

edited

Loading

coot Jul 27, 2018

avieth Jul 20, 2018

coot Jul 25, 2018

coot Jul 25, 2018

avieth Jul 20, 2018

coot Jul 25, 2018

coot Jul 26, 2018

avieth Jul 27, 2018

avieth left a comment

avieth Jul 27, 2018

avieth Jul 27, 2018

coot Jul 27, 2018 •

edited

Loading

coot Jul 27, 2018

[CDEC-439] Cleaning ntp client #3264

[CDEC-439] Cleaning ntp client #3264

Conversation

coot commented Jul 17, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coot commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avieth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avieth Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avieth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coot Jul 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikd Jul 18, 2018 •

edited

Loading

avieth Jul 25, 2018 •

edited

Loading

coot Jul 27, 2018 •

edited

Loading