Expose asynchronous API for connect

[fippo]

This adds a nonblocking API for ICE that will allow the PeerConnection's startTransports to start ICE, not wait for ICE to connect and start DTLS so the DTLS handshake can be piggybacked into the STUN messages earlier.

[jech]

Could you please explain why a non-blocking API is better than using the existing API in a separate goroutine?

(Also, I could be mistaken, but I think that the new API is asynchronous rather than nonblocking.)

[fippo]

API symmetry is one reason, the DTLS API makes a clear distinction between setting up the connection and not returning until the connection is up (Handshake here)

[codecov]

Codecov Report

❌ Patch coverage is 87.87879% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.60%. Comparing base (7aa0886) to head (3b7ab38).

Files with missing lines	Patch %	Lines
transport.go	87.87%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #862      +/-   ##
==========================================
+ Coverage   88.53%   88.60%   +0.06%     
==========================================
  Files          44       44              
  Lines        5540     5564      +24     
==========================================
+ Hits         4905     4930      +25     
+ Misses        440      438       -2     
- Partials      195      196       +1     

Flag	Coverage Δ
go	88.60% <87.87%> (+0.06%)	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jech]

No objection, even though I tend to prefer using multiple goroutines to asynchronous APIs, which I find error-prone.

I've looked at the first commit, and it looks good to me. Minor nits:

• please change the summary of the first commit to say "asynchrounous" instead of "nonblocking";
• please change the StartDial and StartAccept comments to indicate clearly that it returns immediately.

Have you already submitted the code where you use the new API? May I have a look?

I don't understand why the second commit is being submitted as part of the same PR, but in any case I'm not competent to review it.

[jech]

Thanks. The first commit looks good to me.

[fippo]

No objection, even though I tend to prefer using multiple goroutines to asynchronous APIs, which I find error-prone.

I've looked at the first commit, and it looks good to me. Minor nits:

• please change the summary of the first commit to say "asynchrounous" instead of "nonblocking";
• please change the StartDial and StartAccept comments to indicate clearly that it returns immediately.

Done, thank you!

Have you already submitted the code where you use the new API? May I have a look?

https://github.com/fippo/pion-webrtc/blob/snap-sped/icetransport.go#L93
has the intended usage. Currently the iceTransport.Start() only returns when ICE is connected which is kinda unhelpful if one wants to put the DTLS packets into the first few stun packets)

I don't understand why the second commit is being submitted as part of the same PR, but in any case I'm not competent to review it.

githubs continued lack of support for stacked PRs / branches... should vanish once the other PR is merged

[jech]

https://github.com/fippo/pion-webrtc/blob/snap-sped/icetransport.go#L93
has the intended usage. Currently the iceTransport.Start() only returns when ICE is connected which is kinda unhelpful if > one wants to put the DTLS packets into the first few stun packets)

Thanks.

While you wait for this commit to land, you could do something like the following, which would decouple the two commits and avoid the issues with stacked branches:

type connErr struct {
    conn *Conn
    err error
}

func (a *Agent) StartDial(remoteUfrag, remotePwd string) (chan connErr) {
    ch := make(chan connErr)
    go func() {
        conn, err := a.Dial(context.Background(), remoteUfrag, remotePwd)
        ch <- connErr{conn, err}
    }()
    return ch
}

func (a *Agent) AwaitConnect(ch chan connErr) (*Conn, error) {
    ce <- ch
    return ce.conn, ce.err
}

[fippo]

(rebased so should be easier to review now)

[Sean-Der]

@fippo @jech @JoTurk I would like to do a new major version and break APIs to match dtls. We did the same to match crypto/tls

• Make Dial and Accept just return a Controlling or Controlled Agent.
• Add Handshake that blocks until you go to connected
• Read and Write start the handshake automatically.

Less confusing for existing Go devs.

[Sean-Der]

Bad idea?

[JoTurk]

@Sean-Der I agree but I don't think ICE is ready for a new major version yet, i would like us to clean more things first, especially around TCP handling. and srflx gathering, prflx too, and i would like to wait for the final API for multiple path write, and batching (I don't like adding batching as a high level API it should be integrated in pion/transport and ice as low level).

Maybe a new API (just a different name) and mark the current Dial as deprecated until we eventually release a new major version, what do you think?

[fippo]

@fippo @jech @JoTurk I would like to do a new major version and break APIs to match dtls. We did the same to match crypto/tls

• Make Dial and Accept just return a Controlling or Controlled Agent.
• Add Handshake that blocks until you go to connected
• Read and Write start the handshake automatically.

Less confusing for existing Go devs.

this would mostly work (apart from read and write) integration as is? I can add comments capturing that plan which SGTM