Repository: pion/webrtc
Pull request #1748 (2021-04-06 07:45:53)

SCTPTransport: add getAssociation
#### Description
Use assocation() instead of accessing private struct member

member variable `association` in SCTPTransport renamed to `sctpAssociation`

This is a follow-up commit to #1747

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/1748?src=pr&el=h1) Report
> Merging [#1748](https://codecov.io/gh/pion/webrtc/pull/1748?src=pr&el=desc) (b5983ec) into [master](https://codecov.io/gh/pion/webrtc/commit/2f77a28dca4bf0e675de94d8d6077fc92a590df6?el=desc) (2f77a28) will **decrease** coverage by `4.93%`.
> The diff coverage is `n/a`.

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

```diff
@@            Coverage Diff             @@
##           master    #1748      +/-   ##
==========================================
- Coverage   75.12%   70.18%   -4.94%     
==========================================
  Files          80       58      -22     
  Lines        5784     2160    -3624     
==========================================
- Hits         4345     1516    -2829     
+ Misses       1062      540     -522     
+ Partials      377      104     -273     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `?` | |
| wasm | `70.18% <ø> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/1748?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [icetcp.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-aWNldGNwLmdv) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [rtpcodec.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-cnRwY29kZWMuZ28=) | `0.00% <0.00%> (-90.00%)` | :arrow_down: |
| [icecandidatepair.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-aWNlY2FuZGlkYXRlcGFpci5nbw==) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [track\_local.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-dHJhY2tfbG9jYWwuZ28=) | `0.00% <0.00%> (-80.00%)` | :arrow_down: |
| [stats.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-c3RhdHMuZ28=) | `0.00% <0.00%> (-67.86%)` | :arrow_down: |
| [internal/mux/muxfunc.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L211eGZ1bmMuZ28=) | `5.26% <0.00%> (-57.90%)` | :arrow_down: |
| [gathering\_complete\_promise.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-Z2F0aGVyaW5nX2NvbXBsZXRlX3Byb21pc2UuZ28=) | `60.00% <0.00%> (-40.00%)` | :arrow_down: |
| [internal/mux/endpoint.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-aW50ZXJuYWwvbXV4L2VuZHBvaW50Lmdv) | `22.22% <0.00%> (-38.89%)` | :arrow_down: |
| [sdpsemantics.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-c2Rwc2VtYW50aWNzLmdv) | `0.00% <0.00%> (-37.50%)` | :arrow_down: |
| [dtlsrole.go](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree#diff-ZHRsc3JvbGUuZ28=) | `68.96% <0.00%> (-31.04%)` | :arrow_down: |
| ... and [28 more](https://codecov.io/gh/pion/webrtc/pull/1748/diff?src=pr&el=tree-more) | |

------

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

zyxar: or is it better to rename `getAssocation()` as `Assocation()`?
Sean-Der: @zyxar that works for me! I would leave it private, I believe Go will have a conflict then though method+member named `assocation` 
Sean-Der: :fire: 

Love these improvements. Feel free to file any improvements you think would be good. Even if they are big projects we can take them on together.

Patch:
diff --git a/datachannel.go b/datachannel.go
index ea417162..1d948954 100644
--- a/datachannel.go
+++ b/datachannel.go
@@ -104,12 +104,7 @@ func (api *API) newDataChannel(params *DataChannelParameters, log logging.Levele
 
 // open opens the datachannel over the sctp transport
 func (d *DataChannel) open(sctpTransport *SCTPTransport) error {
-	if sctpTransport == nil {
-		return errSCTPNotEstablished
-	}
-	sctpTransport.lock.RLock()
-	association := sctpTransport.association
-	sctpTransport.lock.RUnlock()
+	association := sctpTransport.association()
 	if association == nil {
 		return errSCTPNotEstablished
 	}
diff --git a/sctptransport.go b/sctptransport.go
index 98f3ae9c..6718a615 100644
--- a/sctptransport.go
+++ b/sctptransport.go
@@ -41,7 +41,7 @@ type SCTPTransport struct {
 
 	onErrorHandler func(error)
 
-	association                *sctp.Association
+	sctpAssociation            *sctp.Association
 	onDataChannelHandler       func(*DataChannel)
 	onDataChannelOpenedHandler func(*DataChannel)
 
@@ -112,7 +112,7 @@ func (r *SCTPTransport) Start(remoteCaps SCTPCapabilities) error {
 	r.lock.Lock()
 	defer r.lock.Unlock()
 
-	r.association = sctpAssociation
+	r.sctpAssociation = sctpAssociation
 	r.state = SCTPTransportStateConnected
 
 	go r.acceptDataChannels(sctpAssociation)
@@ -124,15 +124,15 @@ func (r *SCTPTransport) Start(remoteCaps SCTPCapabilities) error {
 func (r *SCTPTransport) Stop() error {
 	r.lock.Lock()
 	defer r.lock.Unlock()
-	if r.association == nil {
+	if r.sctpAssociation == nil {
 		return nil
 	}
-	err := r.association.Close()
+	err := r.sctpAssociation.Close()
 	if err != nil {
 		return err
 	}
 
-	r.association = nil
+	r.sctpAssociation = nil
 	r.state = SCTPTransportStateClosed
 
 	return nil
@@ -320,10 +320,6 @@ func (r *SCTPTransport) State() SCTPTransportState {
 }
 
 func (r *SCTPTransport) collectStats(collector *statsReportCollector) {
-	r.lock.Lock()
-	association := r.association
-	r.lock.Unlock()
-
 	collector.Collecting()
 
 	stats := TransportStats{
@@ -332,6 +328,7 @@ func (r *SCTPTransport) collectStats(collector *statsReportCollector) {
 		ID:        "sctpTransport",
 	}
 
+	association := r.association()
 	if association != nil {
 		stats.BytesSent = association.BytesSent()
 		stats.BytesReceived = association.BytesReceived()
@@ -369,3 +366,13 @@ func (r *SCTPTransport) generateAndSetDataChannelID(dtlsRole DTLSRole, idOut **u
 
 	return &rtcerr.OperationError{Err: ErrMaxDataChannelID}
 }
+
+func (r *SCTPTransport) association() *sctp.Association {
+	if r == nil {
+		return nil
+	}
+	r.lock.RLock()
+	association := r.sctpAssociation
+	r.lock.RUnlock()
+	return association
+}


Test patch:
diff --git a/peerconnection_media_test.go b/peerconnection_media_test.go
index 1c75510f..0245f6b2 100644
--- a/peerconnection_media_test.go
+++ b/peerconnection_media_test.go
@@ -338,11 +338,7 @@ func TestPeerConnection_Media_Disconnected(t *testing.T) {
 			// Assert that DTLS is done by pull remote certificate, don't tear down the PC early
 			for {
 				if len(vp8Sender.Transport().GetRemoteCertificate()) != 0 {
-					pcAnswer.sctpTransport.lock.RLock()
-					haveAssociation := pcAnswer.sctpTransport.association != nil
-					pcAnswer.sctpTransport.lock.RUnlock()
-
-					if haveAssociation {
+					if pcAnswer.sctpTransport.association() != nil {
 						break
 					}
 				}