Merge pull request #1 from evanlinjin/bug/fix-client-close-hangs

Fix hang on Client.Close

pkg/dmsg/README.md

@@ -0,0 +1,134 @@
+# `dmsg`
+
+## Terminology
+
+- **entity -** A service of `dmsg` (typically being part of an executable running on a machine).
+- **entity type -** The type of entity. `dmsg` has three entity types; `dmsg.Server`, `dmsg.Client`, `dmsg.Discovery`.
+- **entry -** A data structure that describes an entity and is stored in `dmsg.Discovery` for entities to access.
+- **frame -** The data unit of the `dmsg` system.
+- **frame type -** The type of `dmsg` frame. There are four frame types; `REQUEST`, `ACCEPT`, `CLOSE`, `FWD`, `ACK`.
+- **connection -** The direct line of duplex communication between a `dmsg.Server` and `dmsg.Client`.
+- **transport -** A line of communication between two `dmsg.Client`s that is proxied via a `dmsg.Server`.
+- **transport ID -** A uint16 value that identifies a transport.
+
+## Frames
+
+Frames have two sections; the header and the payload.
+
+```
+|| FrameType | TransportID | PayloadSize || Payload ||
+|| 1 byte    | 2 bytes     | 2 bytes     || ~ bytes ||
+```
+
+
+
+## Entities
+
+The `dmsg` system is made up of three entity types:
+- `dmsg.Discovery` is a RESTful API that allows `dmsg.Client`s to find remote `dmg.Client`s and `dmsg.Server`s.
+- `dmsg.Server` proxies frames between clients.
+- `dmsg.Client` establishes transports between itself and remote `dmsg.Client`s.
+
+### `dmsg.Discovery`
+
+Only 3 endpoints need to be defined; Get Entry, Post Entry, and Get Available Servers.
+
+#### GET Entry
+
+Obtains a messaging node's entry.
+
+> `GET {domain}/discovery/entries/{public_key}`
+
+**REQUEST**
+
+Header:
+
+```
+Accept: application/json
+```
+
+**RESPONSE**
+
+Possible Status Codes:
+
+- Success (200) - Successfully updated record.
+
+  - Header:
+
+    ```
+    Content-Type: application/json
+    ```
+
+  - Body:
+
+    > JSON-encoded entry.
+
+- Not Found (404) - Entry of public key is not found.
+
+- Unauthorized (401) - invalid signature.
+
+- Internal Server Error (500) - something unexpected happened.
+
+#### POST Entry
+
+Posts an entry and replaces the current entry if valid.
+
+> `POST {domain}/discovery/entries`
+
+**REQUEST**
+
+Header:
+
+```
+Content-Type: application/json
+```
+
+Body:
+
+> JSON-encoded, signed Entry.
+
+**RESPONSE**
+
+Possible Response Codes:
+
+- Success (200) - Successfully registered record.
+- Unauthorized (401) - invalid signature.
+- Internal Server Error (500) - something unexpected happened.
+
+#### GET Available Servers
+
+Obtains a subset of available server entries.
+
+> `GET {domain}/discovery/available_servers`
+
+**REQUEST**
+
+Header:
+
+```
+Accept: application/json
+```
+
+**RESPONSE**
+
+Possible Status Codes:
+
+- Success (200) - Got results.
+
+  - Header:
+
+    ```
+    Content-Type: application/json
+    ```
+
+  - Body:
+
+    > JSON-encoded `[]Entry`.
+
+- Not Found (404) - No results.
+
+- Forbidden (403) - When access is forbidden.
+
+- Internal Server Error (500) - Something unexpected happened.
+
+

pkg/dmsg/testing.md → pkg/dmsg/TESTING.md

@@ -10,8 +10,6 @@ The individual entities of `dmsg` (`dmsg.Client` and `dmsg.Server`), should be c
 
 Note that even though `messaging-discovery` is also considered to be an entity of `dmsg`, however it's mechanics are simple and will not be tested here.
 
-#### Ensure that `Client.Serve()` does not hang
-
 **`failed_accepts_should_not_result_in_hang`**
 
 - Given:
@@ -33,8 +31,6 @@ Note that even though `messaging-discovery` is also considered to be an entity o
 - Then:
   - It should work as expected still.
 
-#### Handling `msg.Server` Failures
-
 **`reconnect_to_server_should_succeed`**
 
 - Given:
@@ -45,7 +41,12 @@ Note that even though `messaging-discovery` is also considered to be an entity o
   - Both clients will automatically reconnect to the server.
   - Transports can be established between clientA and clientB.
 
+**`server_disconnect_should_close_transports`**
+
+- TODO
+
 ### Fuzz testing
 
 We should test the robustness of the system under different conditions and random order of events. These tests should be written consisiting of x-number of servers, clients and a single discovery.
 
+TODO

pkg/dmsg/client.go

@@ -163,15 +163,7 @@ func (c *ClientConn) Serve(ctx context.Context, accept chan<- *Transport) (err e
 	log := c.log.WithField("remoteServer", c.remoteSrv)
 	log.WithField("connCount", incrementServeCount()).Infoln("ServingConn")
 	defer func() {
-		log.WithError(err).WithField("connCount", decrementServeCount()).Infoln("ClosingConn")
-		c.mx.Lock()
-		for _, tp := range c.tps {
-			if tp != nil {
-				go tp.Close() //nolint:errcheck
-			}
-		}
-		_ = c.Conn.Close() //nolint:errcheck
-		c.mx.Unlock()
+		log.WithError(err).WithField("connCount", decrementServeCount()).Infoln("ConnectionClosed")
 		c.wg.Done()
 	}()
 
@@ -259,8 +251,17 @@ func (c *ClientConn) Close() error {
 		closed = true
 		c.log.WithField("remoteServer", c.remoteSrv).Infoln("ClosingConnection")
 		close(c.done)
+		c.mx.Lock()
+		for _, tp := range c.tps {
+			if tp != nil {
+				go tp.Close() //nolint:errcheck
+			}
+		}
+		_ = c.Conn.Close() //nolint:errcheck
+		c.mx.Unlock()
+		c.wg.Wait()
 	})
-	c.wg.Wait()
+
 	if !closed {
 		return ErrClientClosed
 	}

pkg/dmsg/frame.go

@@ -61,11 +61,11 @@ func (ft FrameType) String() string {
 
 // Frame types.
 const (
-	RequestType = FrameType(1)
-	AcceptType  = FrameType(2)
-	CloseType   = FrameType(3)
-	FwdType     = FrameType(10)
-	AckType     = FrameType(11)
+	RequestType = FrameType(0x1)
+	AcceptType  = FrameType(0x2)
+	CloseType   = FrameType(0x3)
+	FwdType     = FrameType(0xa)
+	AckType     = FrameType(0xb)
 )
 
 // Frame is the dmsg data unit.