Repository: pion/turn
Pull request #51 (2019-06-16 13:17:37)

Improve channel_bind code coverage
1. change ChannelBind's ID to turn.ChannelNumber.
2. move channelBindTimeout to StartArguments.

Sean-Der: Fantastic improvement! Nice work :) 

Also if we make sure we are properly tearing down channels I think we can close out https://github.com/pion/turn/issues/27
codecov-io: # [Codecov](https://codecov.io/gh/pion/turn/pull/51?src=pr&el=h1) Report
> Merging [#51](https://codecov.io/gh/pion/turn/pull/51?src=pr&el=desc) into [master](https://codecov.io/gh/pion/turn/commit/5a2e8d7d59afd61b20c70a98714dd76728dab975?src=pr&el=desc) will **increase** coverage by `5.74%`.
> The diff coverage is `46.66%`.

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

```diff
@@            Coverage Diff             @@
##           master      #51      +/-   ##
==========================================
+ Coverage   11.79%   17.53%   +5.74%     
==========================================
  Files          16       16              
  Lines         670      673       +3     
==========================================
+ Hits           79      118      +39     
+ Misses        574      531      -43     
- Partials       17       24       +7
```


| [Impacted Files](https://codecov.io/gh/pion/turn/pull/51?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/server/server.go](https://codecov.io/gh/pion/turn/pull/51/diff?src=pr&el=tree#diff-aW50ZXJuYWwvc2VydmVyL3NlcnZlci5nbw==) | `0% <0%> (ø)` | :arrow_up: |
| [internal/server/turn.go](https://codecov.io/gh/pion/turn/pull/51/diff?src=pr&el=tree#diff-aW50ZXJuYWwvc2VydmVyL3R1cm4uZ28=) | `0% <0%> (ø)` | :arrow_up: |
| [internal/allocation/allocation.go](https://codecov.io/gh/pion/turn/pull/51/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9hbGxvY2F0aW9uLmdv) | `35.45% <66.66%> (+31.81%)` | :arrow_up: |
| [internal/allocation/channel\_bind.go](https://codecov.io/gh/pion/turn/pull/51/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9jaGFubmVsX2JpbmQuZ28=) | `42.85% <75%> (+42.85%)` | :arrow_up: |
| [internal/allocation/permission.go](https://codecov.io/gh/pion/turn/pull/51/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9wZXJtaXNzaW9uLmdv) | `14.28% <0%> (+14.28%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/pion/turn/pull/51?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/turn/pull/51?src=pr&el=footer). Last update [5a2e8d7...8746649](https://codecov.io/gh/pion/turn/pull/51?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

songjiayang: Hi @Sean-Der , review again please.

 I just add a attribute named `permissionLifetime `  to server. I think `ChannelBind` should have the same lifetime as `CreatePermission` lifetime, so it's 5 minutes for default.
songjiayang: LGTM

Patch:
diff --git a/cmd/simple-turn/main.go b/cmd/simple-turn/main.go
index 58cfb50..18a59ec 100644
--- a/cmd/simple-turn/main.go
+++ b/cmd/simple-turn/main.go
@@ -6,6 +6,7 @@ import (
 	"os"
 	"regexp"
 	"strconv"
+	"time"
 
 	"github.com/pion/turn"
 )
@@ -46,9 +47,21 @@ func main() {
 		log.Panic(err)
 	}
 
-	turn.Start(turn.StartArguments{
+	args := turn.StartArguments{
 		Server:  m,
 		Realm:   realm,
 		UDPPort: udpPort,
-	})
+	}
+
+	channelBindTimeoutStr := os.Getenv("CHANNEL_BIND_TIMEOUT")
+	if channelBindTimeoutStr != "" {
+		channelBindTimeout, err := time.ParseDuration(channelBindTimeoutStr)
+		if err != nil {
+			log.Panicf("CHANNEL_BIND_TIMEOUT=%s is an invalid time Duration", channelBindTimeoutStr)
+		}
+
+		args.ChannelBindTimeout = channelBindTimeout
+	}
+
+	turn.Start(args)
 }
diff --git a/internal/allocation/allocation.go b/internal/allocation/allocation.go
index 170820c..9526ef9 100644
--- a/internal/allocation/allocation.go
+++ b/internal/allocation/allocation.go
@@ -80,7 +80,7 @@ func (a *Allocation) GetPermission(addr net.Addr) *Permission {
 
 // AddChannelBind adds a new ChannelBind to the allocation, it also updates the
 // permissions needed for this ChannelBind
-func (a *Allocation) AddChannelBind(c *ChannelBind) error {
+func (a *Allocation) AddChannelBind(c *ChannelBind, lifetime time.Duration) error {
 	// Check that this channel id isn't bound to another transport address, and
 	// that this transport address isn't bound to another channel id.
 	channelByID := a.GetChannelByID(c.ID)
@@ -96,12 +96,12 @@ func (a *Allocation) AddChannelBind(c *ChannelBind) error {
 
 		c.allocation = a
 		a.channelBindings = append(a.channelBindings, c)
-		c.start()
+		c.start(lifetime)
 
 		// Channel binds also refresh permissions.
 		a.AddPermission(&Permission{Addr: c.Peer})
 	} else {
-		channelByID.refresh()
+		channelByID.refresh(lifetime)
 
 		// Channel binds also refresh permissions.
 		a.AddPermission(&Permission{Addr: channelByID.Peer})
@@ -111,7 +111,7 @@ func (a *Allocation) AddChannelBind(c *ChannelBind) error {
 }
 
 // RemoveChannelBind removes the ChannelBind from this allocation by id
-func (a *Allocation) RemoveChannelBind(id uint16) bool {
+func (a *Allocation) RemoveChannelBind(id turn.ChannelNumber) bool {
 	a.channelBindingsLock.Lock()
 	defer a.channelBindingsLock.Unlock()
 
@@ -126,7 +126,7 @@ func (a *Allocation) RemoveChannelBind(id uint16) bool {
 }
 
 // GetChannelByID gets the ChannelBind from this allocation by id
-func (a *Allocation) GetChannelByID(id uint16) *ChannelBind {
+func (a *Allocation) GetChannelByID(id turn.ChannelNumber) *ChannelBind {
 	a.channelBindingsLock.RLock()
 	defer a.channelBindingsLock.RUnlock()
 	for _, cb := range a.channelBindings {
@@ -221,7 +221,7 @@ func (a *Allocation) packetHandler(m *Manager) {
 
 		if channel := a.GetChannelByAddr(&net.UDPAddr{IP: cm.Dst, Port: a.RelayAddr.(*net.UDPAddr).Port}); channel != nil {
 			channelData := make([]byte, 4)
-			binary.BigEndian.PutUint16(channelData[0:], channel.ID)
+			binary.BigEndian.PutUint16(channelData[0:], uint16(channel.ID))
 			binary.BigEndian.PutUint16(channelData[2:], uint16(n))
 			channelData = append(channelData, buffer[:n]...)
 
diff --git a/internal/allocation/channel_bind.go b/internal/allocation/channel_bind.go
index f278319..61e31a4 100644
--- a/internal/allocation/channel_bind.go
+++ b/internal/allocation/channel_bind.go
@@ -4,29 +4,29 @@ import (
 	"fmt"
 	"net"
 	"time"
-)
 
-const channelBindTimeout = time.Duration(10) * time.Minute
+	"github.com/gortc/turn"
+)
 
 // ChannelBind represents a TURN Channel
 // https://tools.ietf.org/html/rfc5766#section-2.5
 type ChannelBind struct {
 	Peer          net.Addr
-	ID            uint16
+	ID            turn.ChannelNumber
 	allocation    *Allocation
 	lifetimeTimer *time.Timer
 }
 
-func (c *ChannelBind) start() {
-	c.lifetimeTimer = time.AfterFunc(channelBindTimeout, func() {
+func (c *ChannelBind) start(lifetime time.Duration) {
+	c.lifetimeTimer = time.AfterFunc(lifetime, func() {
 		if !c.allocation.RemoveChannelBind(c.ID) {
 			fmt.Printf("Failed to remove ChannelBind for %v %x %v \n", c.ID, c.Peer, c.allocation.fiveTuple)
 		}
 	})
 }
 
-func (c *ChannelBind) refresh() {
-	if !c.lifetimeTimer.Reset(channelBindTimeout) {
+func (c *ChannelBind) refresh(lifetime time.Duration) {
+	if !c.lifetimeTimer.Reset(lifetime) {
 		fmt.Printf("Failed to reset ChannelBind timer for %v %x %v \n", c.ID, c.Peer, c.allocation.fiveTuple)
 	}
 }
diff --git a/internal/server/server.go b/internal/server/server.go
index b57ad6e..ff8ece3 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -31,10 +31,11 @@ type Server struct {
 	authHandler        AuthHandler
 	manager            *allocation.Manager
 	reservationManager *allocation.ReservationManager
+	channelBindTimeout time.Duration
 }
 
 // NewServer creates the Pion TURN server
-func NewServer(realm string, a AuthHandler) *Server {
+func NewServer(realm string, channelBindTimeout time.Duration, a AuthHandler) *Server {
 	const maxStunMessageSize = 1500
 	return &Server{
 		packet:             make([]byte, maxStunMessageSize),
@@ -42,6 +43,7 @@ func NewServer(realm string, a AuthHandler) *Server {
 		authHandler:        a,
 		manager:            &allocation.Manager{},
 		reservationManager: &allocation.ReservationManager{},
+		channelBindTimeout: channelBindTimeout,
 	}
 }
 
diff --git a/internal/server/turn.go b/internal/server/turn.go
index bdf17c8..74dc6de 100644
--- a/internal/server/turn.go
+++ b/internal/server/turn.go
@@ -408,12 +408,16 @@ func (s *Server) handleChannelBindRequest(srcAddr, dstAddr net.Addr, m *stun.Mes
 		return errorSend(err, stun.CodeBadRequest)
 	}
 
+	if !channel.Valid() {
+		return errorSend(err, stun.CodeBadRequest)
+	}
+
 	peerAddr := turn.PeerAddress{}
 	if err = peerAddr.GetFrom(m); err != nil {
 		return errorSend(err, stun.CodeBadRequest)
 	}
 
-	err = a.AddChannelBind(&allocation.ChannelBind{ID: uint16(channel), Peer: &net.UDPAddr{IP: peerAddr.IP, Port: peerAddr.Port}})
+	err = a.AddChannelBind(&allocation.ChannelBind{ID: channel, Peer: &net.UDPAddr{IP: peerAddr.IP, Port: peerAddr.Port}}, s.channelBindTimeout)
 	if err != nil {
 		return errorSend(err, stun.CodeBadRequest)
 	}
@@ -431,7 +435,7 @@ func (s *Server) handleChannelData(srcAddr, dstAddr net.Addr, c *turn.ChannelDat
 		return errors.Errorf("No allocation found for %v:%v", srcAddr, dstAddr)
 	}
 
-	channel := a.GetChannelByID(uint16(c.Number))
+	channel := a.GetChannelByID(c.Number)
 	if channel == nil {
 		return errors.Errorf("No channel bind found for %x \n", uint16(c.Number))
 	}
diff --git a/turn.go b/turn.go
index 79d842c..9f2a6f5 100644
--- a/turn.go
+++ b/turn.go
@@ -2,9 +2,10 @@ package turn
 
 import (
 	"fmt"
-
 	"net"
+	"time"
 
+	"github.com/gortc/turn"
 	"github.com/pion/turn/internal/client"
 	"github.com/pion/turn/internal/server"
 )
@@ -21,9 +22,19 @@ type Client interface {
 
 // StartArguments are the arguments for the Pion TURN server
 type StartArguments struct {
-	Server  Server
-	Realm   string
-	UDPPort int
+	Server             Server
+	Realm              string
+	UDPPort            int
+	ChannelBindTimeout time.Duration
+}
+
+// GetChannelBindTimeout is a helper method for StartArguments's default ChannelBindTimeout setting
+func (args StartArguments) GetChannelBindTimeout() time.Duration {
+	if args.ChannelBindTimeout == 0 {
+		return turn.DefaultLifetime
+	}
+
+	return args.ChannelBindTimeout
 }
 
 // ClientArguments are the arguments for the Pion client
@@ -35,12 +46,12 @@ type ClientArguments struct {
 
 // Start the Pion TURN server
 func Start(args StartArguments) {
-	fmt.Println(server.NewServer(args.Realm, args.Server.AuthenticateRequest).Listen("0.0.0.0", args.UDPPort))
+	fmt.Println(server.NewServer(args.Realm, args.GetChannelBindTimeout(), args.Server.AuthenticateRequest).Listen("0.0.0.0", args.UDPPort))
 }
 
 // Create creates the Pion TURN server, but does not call listen
 func Create(args StartArguments) *server.Server {
-	return server.NewServer(args.Realm, args.Server.AuthenticateRequest)
+	return server.NewServer(args.Realm, args.GetChannelBindTimeout(), args.Server.AuthenticateRequest)
 }
 
 // StartClient starts a Pion client


Test patch:
diff --git a/internal/allocation/channel_bind_test.go b/internal/allocation/channel_bind_test.go
new file mode 100644
index 0000000..27beafc
--- /dev/null
+++ b/internal/allocation/channel_bind_test.go
@@ -0,0 +1,48 @@
+package allocation
+
+import (
+	"net"
+	"testing"
+	"time"
+
+	"github.com/gortc/turn"
+)
+
+func TestChannelBind(t *testing.T) {
+	c := newChannelBind(2 * time.Second)
+	if c.allocation.GetChannelByID(c.ID) != c {
+		t.Errorf("GetChannelByID(%d) shouldn't be nil after added to allocation", c.ID)
+	}
+}
+
+func TestChannelBindStart(t *testing.T) {
+	c := newChannelBind(2 * time.Second)
+	time.Sleep(3 * time.Second)
+	if c.allocation.GetChannelByID(c.ID) != nil {
+		t.Errorf("GetChannelByID(%d) should be nil if timeout", c.ID)
+	}
+}
+
+func TestChannelBindReset(t *testing.T) {
+	c := newChannelBind(3 * time.Second)
+	time.Sleep(2 * time.Second)
+	c.refresh(3 * time.Second)
+	time.Sleep(2 * time.Second)
+	if c.allocation.GetChannelByID(c.ID) == nil {
+		t.Errorf("GetChannelByID(%d) shouldn't be nil after refresh", c.ID)
+	}
+}
+
+func newChannelBind(lifetime time.Duration) *ChannelBind {
+	allocation := &Allocation{}
+
+	addr, _ := net.ResolveUDPAddr("udp", "0.0.0.0:0")
+	c := &ChannelBind{
+		ID:   turn.ChannelNumber(turn.MinChannelNumber + 1),
+		Peer: addr,
+	}
+
+	_ = allocation.AddChannelBind(c, lifetime)
+
+	return c
+}