Repository: pion/dtls
Pull request #160 (2019-12-14 12:37:09)

Fix routine leak on close
Read from channel was blocked if closed when there is unread data.
Unblock channel read on close.

https://github.com/pion/webrtc/issues/942 should be fixed by this.
pion/webrtc with this branch is tested at https://github.com/pion/webrtc/issues/942.

codecov-io: # [Codecov](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=h1) Report
> Merging [#160](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=desc) into [master](https://codecov.io/gh/pion/dtls/commit/b4351bb3aded5836d8fa8ae7cc68bf78e1a25552?src=pr&el=desc) will **decrease** coverage by `0.13%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/pion/dtls/pull/160/graphs/tree.svg?width=650&token=bcCB2jB88I&height=150&src=pr)](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #160      +/-   ##
==========================================
- Coverage   73.46%   73.32%   -0.14%     
==========================================
  Files          58       58              
  Lines        3335     3340       +5     
==========================================
- Hits         2450     2449       -1     
- Misses        618      621       +3     
- Partials      267      270       +3
```


| [Impacted Files](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [conn.go](https://codecov.io/gh/pion/dtls/pull/160/diff?src=pr&el=tree#diff-Y29ubi5nbw==) | `79.81% <100%> (-0.72%)` | :arrow_down: |
| [client\_handlers.go](https://codecov.io/gh/pion/dtls/pull/160/diff?src=pr&el=tree#diff-Y2xpZW50X2hhbmRsZXJzLmdv) | `82.59% <0%> (-0.56%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=footer). Last update [b4351bb...3e7b606](https://codecov.io/gh/pion/dtls/pull/160?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/README.md b/README.md
index 9f0887c..56278c3 100644
--- a/README.md
+++ b/README.md
@@ -132,6 +132,7 @@ Check out the **[contributing wiki](https://github.com/pion/webrtc/wiki/Contribu
 * [Hayden James](https://github.com/hjames9)
 * [Jozef Kralik](https://github.com/jkralik)
 * [Robert Eperjesi](https://github.com/epes)
+* [Atsushi Watanabe](https://github.com/at-wat)
 
 ### License
 MIT License - see [LICENSE](LICENSE) for full text
diff --git a/conn.go b/conn.go
index 243db3c..7edeaba 100644
--- a/conn.go
+++ b/conn.go
@@ -82,6 +82,8 @@ type Conn struct {
 
 	bufferedPackets []*packet
 
+	connectionClosed *Closer // Closed on connection close and unblock read
+
 	connErr atomic.Value
 	log     logging.LeveledLogger
 }
@@ -128,6 +130,7 @@ func createConn(nextConn net.Conn, flightHandler flightHandler, handshakeMessage
 	}
 
 	handshakeDoneSignal := NewCloser()
+	connectionClosed := NewCloser()
 
 	c := &Conn{
 		nextConn:                    nextConn,
@@ -156,6 +159,7 @@ func createConn(nextConn net.Conn, flightHandler flightHandler, handshakeMessage
 		decrypted:           make(chan []byte),
 		workerTicker:        time.NewTicker(workerInterval),
 		handshakeDoneSignal: handshakeDoneSignal,
+		connectionClosed:    connectionClosed,
 		log:                 logger,
 	}
 
@@ -624,7 +628,10 @@ func (c *Conn) handleIncomingPacket(buf []byte) (*alert, error) {
 			return &alert{alertLevelFatal, alertUnexpectedMessage}, fmt.Errorf("ApplicationData with epoch of 0")
 		}
 
-		c.decrypted <- content.data
+		select {
+		case c.decrypted <- content.data:
+		case <-c.connectionClosed.Done():
+		}
 	default:
 		return &alert{alertLevelFatal, alertUnexpectedMessage}, fmt.Errorf("unhandled contentType %d", content.contentType())
 	}
@@ -709,6 +716,7 @@ func (c *Conn) stopWithError(err error) {
 	c.workerTicker.Stop()
 
 	c.handshakeDoneSignal.Close()
+	c.connectionClosed.Close()
 }
 
 func (c *Conn) getConnErr() error {


Test patch:
diff --git a/conn_test.go b/conn_test.go
index 8e17a2d..a904e8d 100644
--- a/conn_test.go
+++ b/conn_test.go
@@ -73,6 +73,33 @@ func stressDuplex(t *testing.T) {
 	}
 }
 
+func TestRoutineLeakOnClose(t *testing.T) {
+	// Limit runtime in case of deadlocks
+	lim := test.TimeOut(5 * time.Second)
+	defer lim.Stop()
+
+	// Check for leaking routines
+	report := test.CheckRoutines(t)
+	defer report()
+
+	ca, cb, err := pipeMemory()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if _, err := ca.Write(make([]byte, 100)); err != nil {
+		t.Fatal(err)
+	}
+	if err := cb.Close(); err != nil {
+		t.Fatal(err)
+	}
+	if err := ca.Close(); err != nil {
+		t.Fatal(err)
+	}
+	// Packet is sent, but not read.
+	// inboundLoop routine should not be leaked.
+}
+
 func pipeMemory() (*Conn, *Conn, error) {
 	// In memory pipe
 	ca, cb := net.Pipe()