Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabled accept errors in net-sim testing #3668

Merged
merged 11 commits into from
Mar 18, 2022
+125 −91
Merged

Enabled accept errors in net-sim testing #3668

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1fe3dee
ouroboros-network-framework: added todo
coot Mar 17, 2022
13b9a90
sim-net: removed IOErrThrowOrReturn as an abstraction leak
coot Mar 17, 2022
da66868
sim-net: reset channel when accept errors
coot Mar 17, 2022
7cab8ab
net-sim: added abiAcceptFailure
coot Mar 17, 2022
671fe0b
net-sim: refactor TestError
coot Mar 17, 2022
f3c18cc
net-sim: fix prop_connect_to_accepting_socket
coot Mar 17, 2022
7183908
ouroboros-network-framework: swap bound names
coot Mar 17, 2022
0b33b97
prop_connection_manager_counters: no need to use sayTracer twice
coot Mar 17, 2022
c13a741
sim-net: removed unused trace point
coot Mar 17, 2022
12859bf
net-sim: fixed mismatched addresses
coot Mar 17, 2022
76aee01
Fixed some typos
coot Mar 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion ouroboros-network-framework/src/Ouroboros/Network/ConnectionManager/Core.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1753,7 +1753,7 @@ withConnectionManager ConnectionManagerArguments {
--
-- Key was overwritten in the dictionary (stateVar),
-- so we do not trace anything as it was already traced upon
-- overwritting.
-- overwriting.
else return [ ]


Expand Down
55 changes: 29 additions & 26 deletions ouroboros-network-framework/src/Simulation/Network/Snocket.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ module Simulation.Network.Snocket
, normaliseId
, BearerInfo (..)
, IOErrType (..)
, IOErrThrowOrReturn (..)
, SuccessOrFailure (..)
, TimeoutDetail (..)
, noAttenuation
Expand Down Expand Up @@ -143,7 +142,7 @@ mkConnection tr bearerInfo connId@ConnectionId { localAddress, remoteAddress } =
. STAttenuatedChannelTrace connId
)
`contramap` tr)
( ( WithAddr (Just localAddress) (Just remoteAddress)
( ( WithAddr (Just remoteAddress) (Just localAddress)
. STAttenuatedChannelTrace ConnectionId
{ localAddress = remoteAddress
, remoteAddress = localAddress
Expand Down Expand Up @@ -230,14 +229,6 @@ data IOErrType = IOErrConnectionAborted
deriving (Eq, Show)


-- | The io error can be either thrown by `accept` or return as part of
-- 'AcceptFailure'. Even if our 'Snocket' implementation is faulty, we can
-- verify that the server will behave as expected.
--
data IOErrThrowOrReturn = IOErrThrow
| IOErrReturn
deriving (Eq, Show)

-- | Each bearer info describes outbound and inbound side of a point to
-- point bearer.
--
Expand Down Expand Up @@ -270,7 +261,7 @@ data BearerInfo = BearerInfo
-- which would be caught, and delivered to the application via
-- 'AcceptFailure'.
--
, biAcceptFailures :: !(Maybe (DiffTime, IOErrType, IOErrThrowOrReturn))
, biAcceptFailures :: !(Maybe (DiffTime, IOErrType))

-- | SDU size of the bearer; it will be shared between outbound and inbound
-- sides.
Expand Down Expand Up @@ -359,7 +350,7 @@ instance (Typeable addr, Show addr)


-- | A type class for global IP address scheme. Every node in the simulation
-- has an ephemeral address. Every node in the simulation has an implicity ipv4
-- has an ephemeral address. Every node in the simulation has an implicit ipv4
-- and ipv6 address (if one is not bound by explicitly).
--
class GlobalAddressScheme addr where
Expand Down Expand Up @@ -574,7 +565,6 @@ data SnocketTrace m addr
-- ^ TODO: Document meaning of 'Maybe (Maybe OpenState)'
| STClosingQueue Bool
| STClosedQueue Bool
| STClosedWhenReading
| STAcceptFailure SockType SomeException
| STAccepting
| STAccepted addr
Expand Down Expand Up @@ -1041,7 +1031,7 @@ mkSnocket state tr = Snocket { getLocalAddr
return False

accept_ :: Time
-> Maybe (DiffTime, IOErrType, IOErrThrowOrReturn)
-> Maybe (DiffTime, IOErrType)
-> Accept m (FD m (TestAddress addr))
(TestAddress addr)
accept_ time deltaAndIOErrType = Accept $ do
Expand All @@ -1056,16 +1046,19 @@ mkSnocket state tr = Snocket { getLocalAddr
-- here.
return $ Left ( toException $ invalidError fd
, mbAddr
, Nothing
, mkSockType fd
)
FDConnecting connId _ ->
return $ Left ( toException $ invalidError fd
, Just (localAddress connId)
, Nothing
, mkSockType fd
)
FDConnected connId _ ->
return $ Left ( toException $ invalidError fd
, Just (localAddress connId)
, Nothing
, mkSockType fd
)

Expand All @@ -1080,21 +1073,18 @@ mkSnocket state tr = Snocket { getLocalAddr
case deltaAndIOErrType of
-- the `ctime` is the time when we issued 'accept' not
-- when read something from the queue.
Just (delta, ioErrType, ioErrThrowOrReturn) | delta `addTime` time >= ctime ->
case (ioErrType, ioErrThrowOrReturn) of
(IOErrConnectionAborted, IOErrThrow) ->
throwSTM connectionAbortedError
(IOErrResourceExhausted, IOErrThrow) ->
throwSTM (resourceExhaustedError fd)

(IOErrConnectionAborted, IOErrReturn) ->
Just (delta, ioErrType) | delta `addTime` time >= ctime ->
case ioErrType of
IOErrConnectionAborted ->
return $ Left ( toException connectionAbortedError
, Just localAddress
, Just (connId, cwiChannelLocal cwi)
, mkSockType fd
)
(IOErrResourceExhausted, IOErrReturn) ->
IOErrResourceExhausted ->
return $ Left ( toException $ resourceExhaustedError fd
, Just localAddress
, Just (connId, cwiChannelLocal cwi)
, mkSockType fd
)
_ -> return $ Right ( cwi
Expand All @@ -1103,6 +1093,7 @@ mkSnocket state tr = Snocket { getLocalAddr

FDClosed {} ->
return $ Left ( toException $ invalidError fd
, Nothing
, Nothing
, mkSockType fd
)
Expand All @@ -1125,9 +1116,21 @@ mkSnocket state tr = Snocket { getLocalAddr
)
$ \ result ->
case result of
Left (err, mbLocalAddr, fdType) -> do
traceWith tr (WithAddr (mbLocalAddr) Nothing
(STAcceptFailure fdType err))
Left (err, mbLocalAddr, mbConnIdAndChann, fdType) -> do
uninterruptibleMask_ $
traverse_ (\(connId, chann) -> do
acClose chann
atomically $ modifyTVar
(nsConnections state)
(Map.update
(\conn@Connection { connState } ->
case connState of
FIN -> Nothing
_ -> Just conn { connState = FIN })
(normaliseId connId))
)
mbConnIdAndChann
traceWith tr (WithAddr mbLocalAddr Nothing (STAcceptFailure fdType err))
return (AcceptFailure err, accept_ time deltaAndIOErrType)

Right (chann, connId@ConnectionId { localAddress, remoteAddress }) -> do
Expand Down
81 changes: 27 additions & 54 deletions ouroboros-network-framework/test/Test/Ouroboros/Network/Server2.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,6 @@ import Data.Monoid.Synchronisation (FirstToFinish (..))
import qualified Data.Set as Set
import Data.Typeable (Typeable)
import Data.Void (Void)
import Foreign.C.Error
import qualified GHC.IO.Exception as IO

import Text.Printf

Expand Down Expand Up @@ -119,11 +117,7 @@ import qualified Ouroboros.Network.Snocket as Snocket

import Simulation.Network.Snocket

import Ouroboros.Network.Testing.Data.AbsBearerInfo
(AbsAttenuation (..), AbsBearerInfo (..),
AbsBearerInfoScript (..), AbsDelay (..), AbsSDUSize (..),
AbsSpeed (..), NonFailingAbsBearerInfoScript (..),
absNoAttenuation, toNonFailingAbsBearerInfoScript)
import Ouroboros.Network.Testing.Data.AbsBearerInfo hiding (delay)
import Ouroboros.Network.Testing.Utils (genDelayWithPrecision, nightlyTest)

import Test.Ouroboros.Network.ConnectionManager
Expand Down Expand Up @@ -164,14 +158,10 @@ tests =
, testProperty "bidirectional Sim" prop_bidirectional_Sim
, testProperty "never above hardlimit" prop_never_above_hardlimit
, testGroup "accept errors"
[ testProperty "throw ConnectionAborted"
(unit_server_accept_error IOErrConnectionAborted IOErrThrow)
, testProperty "throw ResourceExhausted"
(unit_server_accept_error IOErrResourceExhausted IOErrThrow)
, testProperty "return ConnectionAborted"
(unit_server_accept_error IOErrConnectionAborted IOErrReturn)
, testProperty "return ResourceExhausted"
(unit_server_accept_error IOErrResourceExhausted IOErrReturn)
[ testProperty "ConnectionAborted"
(unit_server_accept_error IOErrConnectionAborted)
, testProperty "ResourceExhausted"
(unit_server_accept_error IOErrResourceExhausted)
]
]
, testProperty "connection terminated when negotiating"
Expand Down Expand Up @@ -279,7 +269,7 @@ oneshotNextRequests ClientAndServerData {
warmInitiatorRequests,
establishedInitiatorRequests
} = do
-- we pass a `StricTVar` with all the requests to each initiator. This way
-- we pass a `StrictTVar` with all the requests to each initiator. This way
-- the each round (which runs a single instance of `ReqResp` protocol) will
-- use its own request list.
hotRequestsVar <- newTVarIO hotInitiatorRequests
Expand Down Expand Up @@ -1077,7 +1067,7 @@ data MultiNodeScript req peerAddr = MultiNodeScript
deriving (Show)

-- | A sequence of connection events that make up a test scenario for `prop_multinode_Sim_Pruning`.
-- This test optimizes for triggering prunings.
-- This test optimizes for triggering pruning.
data MultiNodePruningScript req = MultiNodePruningScript
{ mnpsAcceptedConnLimit :: AcceptedConnectionsLimit
-- ^ Should yield small values to trigger pruning
Expand Down Expand Up @@ -1285,7 +1275,7 @@ prop_generator_MultiNodeScript (MultiNodeScript script _) =
maxAcceptedConnectionsLimit :: AcceptedConnectionsLimit
maxAcceptedConnectionsLimit = AcceptedConnectionsLimit maxBound maxBound 0

-- | This Script has a percentage of events more favorable to trigger pruning
-- | This Script has a percentage of events more favourable to trigger pruning
-- transitions. And forces a bidirectional connection between each server.
-- It also starts inbound protocols in order to trigger the:
--
Expand All @@ -1300,7 +1290,7 @@ instance Arbitrary req =>
Arbitrary (MultiNodePruningScript req) where
arbitrary = do
Positive len <- scale ((* 2) . (`div` 3)) arbitrary
-- NOTE: Although we still do not enforce the configured hardlimit to be
-- NOTE: Although we still do not enforce the configured hard limit to be
-- strictly positive. We assume that the hard limit is always bigger than
-- 0.
Small hardLimit <- (`div` 10) <$> arbitrary
Expand Down Expand Up @@ -1493,7 +1483,7 @@ multinodeExperiment
-> AcceptedConnectionsLimit
-> MultiNodeScript req peerAddr
-> m ()
multinodeExperiment inboundTrTracer trTracer cmTracer inboundTracer
multinodeExperiment inboundTrTracer trTracer inboundTracer cmTracer
snocket addrFamily serverAddr accInit
dataFlow0 acceptedConnLimit
(MultiNodeScript script _) =
Expand Down Expand Up @@ -1623,7 +1613,7 @@ multinodeExperiment inboundTrTracer trTracer cmTracer inboundTracer
Duplex ->
Job ( withBidirectionalConnectionManager
name simTimeouts
inboundTrTracer trTracer inboundTracer cmTracer
inboundTrTracer trTracer cmTracer inboundTracer
snocket fd (Just localAddr) serverAcc
(mkNextRequests connVar)
timeLimitsHandshake
Expand All @@ -1645,7 +1635,7 @@ multinodeExperiment inboundTrTracer trTracer cmTracer inboundTracer
(show name)
Unidirectional ->
Job ( withInitiatorOnlyConnectionManager
name simTimeouts trTracer inboundTracer snocket (Just localAddr)
name simTimeouts trTracer cmTracer snocket (Just localAddr)
(mkNextRequests connVar)
timeLimitsHandshake
acceptedConnLimit
Expand Down Expand Up @@ -1859,7 +1849,7 @@ instance Arbitrary ArbDataFlow where
data ActivityType
= IdleConn

-- | Active connections are onces that reach any of the state:
-- | Active connections are once that reach any of the state:
--
-- - 'InboundSt'
-- - 'OutobundUniSt'
Expand Down Expand Up @@ -2441,9 +2431,9 @@ prop_connection_manager_counters serverAcc (ArbDataFlow dataFlow)
-- connections.
--
-- TODO: we are computing upper bound of contribution of each
-- address seprately. This avoids tracking timing information of
-- events, which is less acurate but it might be less fragile. We
-- should investiage if it's possible to make accurate and robust
-- address separately. This avoids tracking timing information of
-- events, which is less accurate but it might be less fragile. We
-- should investigate if it's possible to make accurate and robust
-- time series of counter changes.
connectionManagerCounters =
foldMap' id
Expand Down Expand Up @@ -2528,10 +2518,7 @@ prop_connection_manager_counters serverAcc (ArbDataFlow dataFlow)
else (a, b, c + d - a)

networkStateTracer getState =
sayTracer
<> (Tracer $ \_ -> do
state <- getState
traceM state)
Tracer $ \_ -> getState >>= traceM

sim :: IOSim s ()
sim = do
Expand Down Expand Up @@ -2607,17 +2594,17 @@ prop_timeouts_enforced serverAcc (ArbDataFlow dataFlow)
-- verifyTimeouts checks that in all \tau transition states the timeout is
-- respected. It does so by checking the stream of abstract transitions
-- paired with the time they happened, for a given connection; and checking
-- that transitions from \tau states to any other happens withing the correct
-- that transitions from \tau states to any other happens within the correct
-- timeout bounds. One note is that for the example
-- InboundIdleState^\tau -> OutboundState^\tau -> OutboundState sequence
-- The first transition would be fine, but for the second we need the time
-- when we transitioned into InboundIdleState and not OutboundState.
--
verifyTimeouts :: Maybe (AbstractState, Time)
-- ^ Map of first occurence of a given \tau state
-- ^ Map of first occurrence of a given \tau state
-> [(Time , AbstractTransitionTrace SimAddr)]
-- ^ Stream of abstract transitions for a given connection
-- paired with the time it ocurred
-- paired with the time it occurred
-> Property
verifyTimeouts state [] =
counterexample
Expand Down Expand Up @@ -3414,15 +3401,15 @@ prop_never_above_hardlimit serverAcc

-- | Checks that the server re-throws exceptions returned by an 'accept' call.
--
unit_server_accept_error :: IOErrType -> IOErrThrowOrReturn -> Property
unit_server_accept_error ioErrType ioErrThrowOrReturn =
unit_server_accept_error :: IOErrType -> Property
unit_server_accept_error ioErrType =
runSimOrThrow sim
where
-- The following attenuation make sure that the `accept` call will throw.
--
bearerAttenuation :: BearerInfo
bearerAttenuation =
noAttenuation { biAcceptFailures = Just (0, ioErrType, ioErrThrowOrReturn) }
noAttenuation { biAcceptFailures = Just (0, ioErrType) }

sim :: IOSim s Property
sim = handle (\e -> return $ case fromException e of
Expand Down Expand Up @@ -3476,30 +3463,15 @@ unit_server_accept_error ioErrType ioErrThrowOrReturn =
Nothing -> counterexample "server did not throw"
(ioErrType == IOErrConnectionAborted)
Just (Right _) -> counterexample "unexpected value" False
Just (Left e) | IOErrReturn <- ioErrThrowOrReturn
, Just (err :: IOError) <- fromException e
-- any IO exception which is not
-- 'ECONNABORTED' is ok
-- TODO: use isECONNABORTED function
-> case IO.ioe_errno err of
Just errno ->
case eCONNABORTED of
Errno errno' ->
counterexample
("unexpected error " ++ show e) $
errno /= errno'
Nothing ->
property True

-- If we throw exceptions, any IO exception
Just (Left e) -- If we throw exceptions, any IO exception
-- can go through; the check for
-- 'ECONNABORTED' relies on that. It is
-- a bug in a snocket implementation if it
-- throws instead of returns io errors.
| Just (_ :: IOError) <- fromException e
-> property True

| otherwise
| otherwise
-> counterexample ("unexpected error " ++ show e)
False
)
Expand Down Expand Up @@ -3602,6 +3574,7 @@ unit_connection_terminated_when_negotiating =
, abiOutboundAttenuation = NoAttenuation FastSpeed
, abiInboundWriteFailure = Nothing
, abiOutboundWriteFailure = Just 3
, abiAcceptFailure = Nothing
, abiSDUSize = LargeSDU
}
multiNodeScript =
Expand Down Expand Up @@ -3631,7 +3604,7 @@ unit_connection_terminated_when_negotiating =
multiNodeScript


-- | Split 'AbstractTransitionTrace' into seprate connections. This relies on
-- | Split 'AbstractTransitionTrace' into separate connections. This relies on
-- the property that every connection is terminated with 'UnknownConnectionSt'.
-- This property is verified by 'verifyAbstractTransitionOrder'.
--
Expand Down
Loading