@@ -248,61 +248,29 @@ data ValidLocalEndPointState = ValidLocalEndPointState
248248 , _nextRemoteId :: !Int
249249 }
250250
251−-- A remote endpoint has incoming and outgoing connections, and when the total
252−-- number of connections (that is, the 'remoteRefCount') drops to zero we want
253−-- to close the TCP connection to the endpoint.
251+-- Invariants for dealing with remote endpoints:
254252--
255−-- What we need to avoid, however, is a situation with two concurrent threads
256−-- where one closes the last (incoming or outgoing) connection, initiating the
257−-- process of closing the connection, while another requests (but does not yet
258−-- have) a new connection.
253+-- INV-SEND: Whenever we send data the remote endpoint must be locked (to avoid
254+-- interleaving bits of payload).
259255--
260−-- We therefore insist that:
261−--
262−-- 1. All operations that change the state of the endpoint (ask for a new
263−-- connection, close a connection, close the endpoint completely) are
264−-- serialized (that is, they take the contents of the MVar containing the
265−-- endpoint state before starting and don't put the updated contents back
266−-- until they have completed).
267−-- 2. Writing to ('apiSend') or reading from (in 'handleIncomingMessages') must
268−-- maintain the invariant that the connection they are writing to/reading
269−-- from *must* be "included" in the 'remoteRefCount'.
270−-- 3. Since every endpoint is associated with a single socket, we regard writes
271−-- that endpoint a state change too (i.e., we take the MVar before the write
272−-- and put it back after). The reason is that we don't want to "scramble" the
273−-- output of multiple concurrent writes (either from an explicit 'send' or
274−-- the writes for control messages).
275−--
276−-- Of course, "serialize" does not mean that we want for the remote endpoint to
277−-- reply. "Send" takes the mvar, sends to the endpoint (asynchronously), and
278−-- then puts the mvar back, without waiting for the endpoint to receive the
279−-- message. Similarly, when requesting a new connection, we take the mvar,
280−-- tentatively increment the reference count, send the control request, and
281−-- then put the mvar back. When the remote host responds to the new connection
282−-- request we might have to do another state change (reduce the refcount) if
283−-- the connection request was refused but we don't want to increment the ref
284−-- count only after the remote host acknowledges the request because then a
285−-- concurrent 'close' might actually attempt to close the socket.
286−--
287−-- Since we don't do concurrent reads from the same socket we don't need to
288−-- take the lock when reading from the socket.
289−--
290−-- Invariants:
291−--
292−-- INV-CLOSE: Whenever we put an endpoint in closed state we remove that
256+-- INV-CLOSE: Local endpoints should never point to remote endpoint in closed
257+-- state. Whenever we put an endpoint in closed state we remove that
293258-- endpoint from localConnections first, so that if a concurrent thread reads
294−-- the mvar, finds RemoteEndPointClosed, and then looks up the endpoint in
259+-- the MVar, finds RemoteEndPointClosed, and then looks up the endpoint in
295260-- localConnections it is guaranteed to either find a different remote
296−-- endpoint, or else none at all.
297−-- INV-RESOLVE: Whenever we move a endpoint from Closing to Closed state, we
298−-- signal on the corresponding MVar only *after* the endpoint has been put in
299−-- Closed state. This way when to threads try to resolve they don't both
300−-- attempt to write to the "resolved" MVar. TODO: Make sure that this
301−-- invariant is adhered too.
261+-- endpoint, or else none at all (if we don't insist in this order some
262+-- threads might start spinning).
263+--
264+-- INV-RESOLVE: We should only signal on 'resolved' while the remote endpoint is
265+-- locked, and the remote endpoint must be in Valid or Closed state once
266+-- unlocked. This guarantees that there will not be two threads attempting to
267+-- both signal on 'resolved'.
268+--
302269-- INV-LOST: If a send or recv fails, or a socket is closed unexpectedly, we
303270-- first put the remote endpoint in Closing or Closed state, and then send a
304271-- EventConnectionLost event. This guarantees that we only send this event
305272-- once.
273+--
306274-- INV-CLOSING: An endpoint in closing state is for all intents and purposes
307275-- closed; that is, we shouldn't do any 'send's on it (although 'recv' is
308276-- acceptable, of course -- as we are waiting for the remote endpoint to
@@ -368,8 +336,6 @@ data TransportInternals = TransportInternals
368336--------------------------------------------------------------------------------
369337
370338-- | Create a TCP transport
371−--
372−-- TODOs: deal with hints
373339createTransport :: N.HostName
374340 -> N.ServiceName
375341 -> IO (Either IOException Transport)
@@ -1122,8 +1088,8 @@ modifyRemoteState (ourEndPoint, theirEndPoint) match =
11221088 st <- takeMVar theirState
11231089 case st of
11241090 RemoteEndPointClosing resolved vst -> do
1125− putMVar theirState RemoteEndPointClosed
11261091 putMVar resolved ()
1092+ putMVar theirState RemoteEndPointClosed
11271093 return . Just . IntSet.elems $ vst ^. remoteIncoming
11281094 RemoteEndPointClosed -> do
11291095 putMVar theirState RemoteEndPointClosed
@@ -1373,23 +1339,28 @@ handleIncomingMessages (ourEndPoint, theirEndPoint) = do
13731339 -- Attempt to reply (but don't insist)
13741340 tryIO $ sendOn vst' [encodeInt32 CloseSocket]
13751341 resolved <- newEmptyMVar
1376− return (RemoteEndPointClosing resolved vst', Just resolved)
1342+ return (RemoteEndPointClosing resolved vst', True)
13771343 else
1378− return (RemoteEndPointValid vst', Nothing)
1379− RemoteEndPointClosing resolved _ ->
1380− return (st, Just resolved)
1344+ return (RemoteEndPointValid vst', False)
1345+ RemoteEndPointClosing _ _ ->
1346+ return (st, True)
13811347 _ ->
13821348 error "handleIncomingConnections RELY violation"
1349+
1350+ when canClose $ do
1351+ removeRemoteEndPoint (ourEndPoint, theirEndPoint)
1352+ modifyMVar_ theirState $ \st -> do
1353+ case st of
1354+ RemoteEndPointClosing resolved _ ->
1355+ putMVar resolved ()
1356+ RemoteEndPointClosed ->
1357+ return ()
1358+ _ ->
1359+ fail "Impossible case"
1360+ return RemoteEndPointClosed
1361+ tryCloseSocket sock
13831362
1384− case canClose of
1385− Nothing ->
1386− return False
1387− Just resolved -> do
1388− removeRemoteEndPoint (ourEndPoint, theirEndPoint)
1389− modifyMVar_ theirState $ return . const RemoteEndPointClosed
1390− tryCloseSocket sock
1391− putMVar resolved ()
1392− return True
1363+ return canClose
13931364
13941365 -- Read a message and output it on the endPoint's channel By rights we
13951366 -- should verify that the connection ID is valid, but this is unnecessary