Remove global state for ICE TCP

[jeremija]

This addresses a few points issue of #245:

• Take a net.Listener instead of having global state
• Expose a net.PacketConn based API, Agent Expose a net.TCPMux based API

Also, the unused CloseChannel was removed from tcp_mux.go.

Closes #253.

[jeremija]

@Sean-Der I know we talked about how we should avoid nil values, but I couldn't figure out of a better solution now that TCPMux is optional. I'd be happy to hear if you have a solution that allows us to never have a nil value in Agent.tcpMux.

[codecov]

Codecov Report

Merging #254 into master will decrease coverage by 0.13%.
The diff coverage is 80.55%.

@@            Coverage Diff             @@
##           master     #254      +/-   ##
==========================================
- Coverage   80.50%   80.36%   -0.14%     
==========================================
  Files          32       31       -1     
  Lines        2503     2450      -53     
==========================================
- Hits         2015     1969      -46     
+ Misses        349      342       -7     
  Partials      139      139              

Flag	Coverage Δ
#go	80.36% <80.55%> (-0.02%)	⬇️
#wasm	23.18% <52.77%> (-1.15%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
agent_config.go	85.07% <ø> (ø)
gather.go	71.82% <33.33%> (+2.31%)	⬆️
tcp_packet_conn.go	69.15% <66.66%> (-2.14%)	⬇️
tcp_mux.go	76.25% <84.61%> (-2.42%)	⬇️
agent.go	83.53% <100.00%> (+0.25%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1f59642...d8f107b. Read the comment docs.

[Sean-Der]

Absolutely amazing, this is so exciting to see a red diff :) couple ideas/points (push back if you disagree)

• I would make TCPMux an interface. That way we can mock it out in the future if we need.
• Maybe do a invalidTCPMux instead of a nil pointer? Just so much easier to be resilient when stuff returns an err (instead of segv the whole process)

[jeremija]

Sounds good, will try to do it over the weekend! An alternative would be to make the pointer receiver methods check whether the pointer is nil, but I like your solution better.

[jeremija]

I think we will probably need a method that would allow us to close a TCP connection eventually. Something like:

func (t *TCPMux) CloseConn(ufrag string, raddr net.Addr) error

WDYT?

[Sean-Der]

This is something a user will need to do?

Right now every gathered candidate gets closed internally, so ufrags shouldn't pile up? (I think)

sorry will check on my laptop soon :)

[jeremija]

No worries, I think I'm just getting a little confused :) So when I look at this statement from https://tools.ietf.org/html/rfc6544:

When ICE processing for a media stream completes, each agent SHOULD close all TCP connections (that were opened due to this ICE session) except the ones between the candidate pairs selected by ICE.

In the context of our host (listening tcp) candidate, does this mean we don't have to manually close the remote TCP connections? Is this because these (unselected) inbound TCP connections will actually be handled (closed) internally as remote candidates?

[jeremija]

I have addressed both of your points:

• I would make TCPMux an interface. That way we can mock it out in the future if we need.
• Maybe do a invalidTCPMux instead of a nil pointer? Just so much easier to be resilient when stuff returns an err (instead of segv the whole process)

and I'm going to merge this since you already approved it. If you have any other concerns I'd be happy to push a fix later. I'll tag this release and push an update to pion/webrtc. Excited to finally release this feature :)