Repository: pion/turn
Pull request #55 (2019-06-21 11:07:21)

Allocation lifetime refactor
1. lifetime from uint32 to time.Duration.
2. turn.maximumLifetime from 3600 to one hour.


codecov-io: # [Codecov](https://codecov.io/gh/pion/turn/pull/55?src=pr&el=h1) Report
> Merging [#55](https://codecov.io/gh/pion/turn/pull/55?src=pr&el=desc) into [master](https://codecov.io/gh/pion/turn/commit/93a39c0090f2c6f2eae6f1a6592bcc98071d93fe?src=pr&el=desc) will **increase** coverage by `1.08%`.
> The diff coverage is `53.33%`.

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

```diff
@@            Coverage Diff             @@
##           master      #55      +/-   ##
==========================================
+ Coverage   17.42%   18.51%   +1.08%     
==========================================
  Files          16       16              
  Lines         677      675       -2     
==========================================
+ Hits          118      125       +7     
+ Misses        535      526       -9     
  Partials       24       24
```


| [Impacted Files](https://codecov.io/gh/pion/turn/pull/55?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [internal/server/server.go](https://codecov.io/gh/pion/turn/pull/55/diff?src=pr&el=tree#diff-aW50ZXJuYWwvc2VydmVyL3NlcnZlci5nbw==) | `0% <ø> (ø)` | :arrow_up: |
| [internal/allocation/allocation.go](https://codecov.io/gh/pion/turn/pull/55/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9hbGxvY2F0aW9uLmdv) | `35.45% <0%> (ø)` | :arrow_up: |
| [internal/allocation/allocation\_manager.go](https://codecov.io/gh/pion/turn/pull/55/diff?src=pr&el=tree#diff-aW50ZXJuYWwvYWxsb2NhdGlvbi9hbGxvY2F0aW9uX21hbmFnZXIuZ28=) | `50.84% <50%> (ø)` | :arrow_up: |
| [internal/server/turn.go](https://codecov.io/gh/pion/turn/pull/55/diff?src=pr&el=tree#diff-aW50ZXJuYWwvc2VydmVyL3R1cm4uZ28=) | `2.85% <63.63%> (+2.85%)` | :arrow_up: |

------

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


Patch:
diff --git a/internal/allocation/allocation.go b/internal/allocation/allocation.go
index 9526ef9..f1ae4bc 100644
--- a/internal/allocation/allocation.go
+++ b/internal/allocation/allocation.go
@@ -150,14 +150,14 @@ func (a *Allocation) GetChannelByAddr(addr net.Addr) *ChannelBind {
 }
 
 // Refresh updates the allocations lifetime
-func (a *Allocation) Refresh(lifetime uint32) {
+func (a *Allocation) Refresh(lifetime time.Duration) {
 	if lifetime == 0 {
 		if !a.lifetimeTimer.Stop() {
 			fmt.Printf("Failed to stop allocation timer for %v \n", a.fiveTuple)
 		}
 		return
 	}
-	if !a.lifetimeTimer.Reset(time.Duration(lifetime) * time.Second) {
+	if !a.lifetimeTimer.Reset(lifetime) {
 		fmt.Printf("Failed to reset allocation timer for %v \n", a.fiveTuple)
 	}
 }
@@ -182,6 +182,7 @@ func (a *Allocation) Close() error {
 		c.lifetimeTimer.Stop()
 	}
 	a.channelBindingsLock.RUnlock()
+
 	return a.RelaySocket.Close()
 }
 
diff --git a/internal/allocation/allocation_manager.go b/internal/allocation/allocation_manager.go
index 66e1bae..edbd11d 100644
--- a/internal/allocation/allocation_manager.go
+++ b/internal/allocation/allocation_manager.go
@@ -43,7 +43,7 @@ func (m *Manager) Close() error {
 }
 
 // CreateAllocation creates a new allocation and starts relaying
-func (m *Manager) CreateAllocation(fiveTuple *FiveTuple, turnSocket ipnet.PacketConn, requestedPort int, lifetime uint32) (*Allocation, error) {
+func (m *Manager) CreateAllocation(fiveTuple *FiveTuple, turnSocket ipnet.PacketConn, requestedPort int, lifetime time.Duration) (*Allocation, error) {
 	if fiveTuple == nil {
 		return nil, errors.Errorf("Allocations must not be created with nil FivTuple")
 	}
@@ -82,7 +82,7 @@ func (m *Manager) CreateAllocation(fiveTuple *FiveTuple, turnSocket ipnet.Packet
 	a.RelaySocket = conn
 	a.RelayAddr = listener.LocalAddr()
 
-	a.lifetimeTimer = time.AfterFunc(time.Duration(lifetime)*time.Second, func() {
+	a.lifetimeTimer = time.AfterFunc(lifetime, func() {
 		if err := listener.Close(); err != nil {
 			fmt.Printf("Failed to close listener for %v \n", a.fiveTuple)
 		}
diff --git a/internal/server/server.go b/internal/server/server.go
index ff8ece3..3d93d48 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -34,9 +34,10 @@ type Server struct {
 	channelBindTimeout time.Duration
 }
 
+const maxStunMessageSize = 1500
+
 // NewServer creates the Pion TURN server
 func NewServer(realm string, channelBindTimeout time.Duration, a AuthHandler) *Server {
-	const maxStunMessageSize = 1500
 	return &Server{
 		packet:             make([]byte, maxStunMessageSize),
 		realm:              realm,
diff --git a/internal/server/turn.go b/internal/server/turn.go
index 74dc6de..6bc1c06 100644
--- a/internal/server/turn.go
+++ b/internal/server/turn.go
@@ -16,7 +16,7 @@ import (
 )
 
 const (
-	maximumLifetime = uint32(3600) // https://tools.ietf.org/html/rfc5766#section-6.2 defines 3600 recommendation
+	maximumLifetime = time.Hour // https://tools.ietf.org/html/rfc5766#section-6.2 defines 3600 seconds recommendation
 )
 
 type curriedSend func(class stun.MessageClass, method stun.Method, transactionID [stun.TransactionIDSize]byte, attrs ...stun.Setter) error
@@ -204,15 +204,8 @@ func (s *Server) handleAllocateRequest(srcAddr, dstAddr net.Addr, m *stun.Messag
 	//    attribute follow the specification in [RFC5389].
 	// Check current usage vs redis usage of other servers
 	// if bad, redirect { stun.AttrErrorCode, 300 }
-	lifetimeDuration := turn.DefaultLifetime
-	var lifetime turn.Lifetime
-	if err = lifetime.GetFrom(m); err == nil {
-		if uint32(lifetime.Duration/time.Second) < maximumLifetime {
-			lifetimeDuration = lifetime.Duration
-		}
-	}
-
-	a, err := s.manager.CreateAllocation(fiveTuple, s.connection, requestedPort, uint32(lifetimeDuration/time.Second))
+	lifetimeDuration := allocationLifeTime(m)
+	a, err := s.manager.CreateAllocation(fiveTuple, s.connection, requestedPort, lifetimeDuration)
 	if err != nil {
 		return respondWithError(err, messageIntegrity, stun.CodeInsufficientCapacity)
 	}
@@ -283,14 +276,8 @@ func (s *Server) handleRefreshRequest(srcAddr, dstAddr net.Addr, m *stun.Message
 		return errors.Errorf("No allocation found for %v:%v", srcAddr, dstAddr)
 	}
 
-	lifetimeDuration := turn.DefaultLifetime
-	var lifetime turn.Lifetime
-	if err = lifetime.GetFrom(m); err == nil {
-		if uint32(lifetime.Duration*time.Second) < maximumLifetime {
-			lifetimeDuration = lifetime.Duration
-		}
-	}
-	a.Refresh(uint32(lifetimeDuration * time.Second))
+	lifetimeDuration := allocationLifeTime(m)
+	a.Refresh(lifetimeDuration)
 
 	return curriedSend(stun.ClassSuccessResponse, stun.MethodRefresh, m.TransactionID,
 		&turn.Lifetime{
@@ -451,3 +438,16 @@ func (s *Server) handleChannelData(srcAddr, dstAddr net.Addr, c *turn.ChannelDat
 
 	return nil
 }
+
+func allocationLifeTime(m *stun.Message) time.Duration {
+	lifetimeDuration := turn.DefaultLifetime
+
+	var lifetime turn.Lifetime
+	if err := lifetime.GetFrom(m); err == nil {
+		if lifetime.Duration < maximumLifetime {
+			lifetimeDuration = lifetime.Duration
+		}
+	}
+
+	return lifetimeDuration
+}


Test patch:
diff --git a/internal/allocation/allocation_manager_test.go b/internal/allocation/allocation_manager_test.go
index e54519d..53377ab 100644
--- a/internal/allocation/allocation_manager_test.go
+++ b/internal/allocation/allocation_manager_test.go
@@ -5,6 +5,7 @@ import (
 	"net"
 	"testing"
 
+	"github.com/gortc/turn"
 	"github.com/pion/turn/internal/ipnet"
 )
 
@@ -40,10 +41,10 @@ func TestAllocation(t *testing.T) {
 // test invalid Allocation creations
 func subTestCreateInvalidAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := &Manager{}
-	if a, err := m.CreateAllocation(nil, turnSocket, 0, 5000); a != nil || err == nil {
+	if a, err := m.CreateAllocation(nil, turnSocket, 0, turn.DefaultLifetime); a != nil || err == nil {
 		t.Errorf("Illegally created allocation with nil FiveTuple")
 	}
-	if a, err := m.CreateAllocation(randomFiveTuple(), nil, 0, 5000); a != nil || err == nil {
+	if a, err := m.CreateAllocation(randomFiveTuple(), nil, 0, turn.DefaultLifetime); a != nil || err == nil {
 		t.Errorf("Illegally created allocation with nil turnSocket")
 	}
 	if a, err := m.CreateAllocation(randomFiveTuple(), turnSocket, 0, 0); a != nil || err == nil {
@@ -55,7 +56,7 @@ func subTestCreateInvalidAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 func subTestCreateAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := &Manager{}
 	fiveTuple := randomFiveTuple()
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, 5000); a == nil || err != nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a == nil || err != nil {
 		t.Errorf("Failed to create allocation %v %v", a, err)
 	}
 
@@ -68,11 +69,11 @@ func subTestCreateAllocation(t *testing.T, turnSocket ipnet.PacketConn) {
 func subTestCreateAllocationDuplicateFiveTuple(t *testing.T, turnSocket ipnet.PacketConn) {
 	m := &Manager{}
 	fiveTuple := randomFiveTuple()
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, 5000); a == nil || err != nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a == nil || err != nil {
 		t.Errorf("Failed to create allocation %v %v", a, err)
 	}
 
-	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, 5000); a != nil || err == nil {
+	if a, err := m.CreateAllocation(fiveTuple, turnSocket, 0, turn.DefaultLifetime); a != nil || err == nil {
 		t.Errorf("Was able to create allocation with same FiveTuple twice")
 	}
 }
diff --git a/internal/server/turn_test.go b/internal/server/turn_test.go
new file mode 100644
index 0000000..d1e043e
--- /dev/null
+++ b/internal/server/turn_test.go
@@ -0,0 +1,47 @@
+package server
+
+import (
+	"testing"
+	"time"
+
+	"github.com/gortc/turn"
+	"github.com/pion/stun"
+)
+
+func TestAllocationLifeTime(t *testing.T) {
+	lifetime := turn.Lifetime{
+		Duration: 5 * time.Second,
+	}
+
+	m := &stun.Message{}
+	lifetimeDuration := allocationLifeTime(m)
+
+	if lifetimeDuration != turn.DefaultLifetime {
+		t.Errorf("Allocation lifetime should be default time duration")
+	}
+
+	if err := lifetime.AddTo(m); err != nil {
+		t.Errorf("Lifetime add to message failed: %v", err)
+	}
+
+	lifetimeDuration = allocationLifeTime(m)
+	if lifetimeDuration != lifetime.Duration {
+		t.Errorf("Expect lifetimeDuration is %s, but %s", lifetime.Duration, lifetimeDuration)
+	}
+
+	// If lifetime is bigger than maximumLifetime
+	{
+		lifetime := turn.Lifetime{
+			Duration: maximumLifetime * 2,
+		}
+
+		m2 := &stun.Message{}
+		_ = lifetime.AddTo(m2)
+
+		lifetimeDuration := allocationLifeTime(m2)
+		if lifetimeDuration != turn.DefaultLifetime {
+			t.Errorf("Expect lifetimeDuration is %s, but %s", turn.DefaultLifetime, lifetimeDuration)
+		}
+	}
+
+}