Support context.Context based cancellation

[at-wat]

DialWithContext(), ClientWithContext() and ServerWithContext()
are added. Instead, ConnectTimeout is dropped from Config.

Reference issue

Closes #189

[codecov-io]

Codecov Report

Merging #191 into master will decrease coverage by 0.12%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #191      +/-   ##
==========================================
- Coverage   75.36%   75.23%   -0.13%     
==========================================
  Files          63       63              
  Lines        3540     3550      +10     
==========================================
+ Hits         2668     2671       +3     
- Misses        586      591       +5     
- Partials      286      288       +2

Flag	Coverage Δ
#go	75.23% <84.61%> (-0.02%)	⬇️
#wasm	69.57% <76.92%> (-0.23%)	⬇️

Impacted Files	Coverage Δ
listener.go	57.89% <ø> (ø)	⬆️
resume.go	62.5% <100%> (ø)	⬆️
config.go	96% <83.33%> (-4%)	⬇️
conn.go	80.78% <84.21%> (-1.08%)	⬇️
internal/net/dpipe/dpipe.go	94.11% <0%> (-2.36%)	⬇️
client_handlers.go	78.16% <0%> (-0.54%)	⬇️
cipher_suite.go	93.04% <0%> (+3.47%)	⬆️

---

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 34883a7...821a2c5. Read the comment docs.

[at-wat]

This function type suggest user to use cancelable contexts.

[Sean-Der]

I am fine removing this entirely! ping @daenney make sure I am not making a bad decision :)

• Context with a deadline if they still need deadlines.
• We don't support Dialer, so not like we are breaking any APIs (Dialer also doesn't support server so will not work for us)

[at-wat]

I added this to make it possible to customize the behavior of accepting connection in dtls.listener.

As Listen() blocks until incoming connection and return established dtls.Conn, deadline timer must be initialized at the time the first packet, that triggers dtls handshake, arrives.

Adding this makes it easy to implement both handshake timeout and graceful shutdown of server like:

func DTLSServer(ctx context.Context) {
	l, err := dtls.Listen(..., &dtls.Config{
		return context.WithTimeout(ctx, 10*time.Second)
	})
	for {
		conn, err := l.Accept()
		...
	}
}

[daenney]

Thanks for the ping. I think this makes sense, and the fact that when not explicitly provided we inject a default context with a proper timeout makes me happy.

[at-wat]

@daenney @Sean-Der @jkralik please take a look when you have time!

[at-wat]

Increased context related test coverage.

[Sean-Der]

Mind making this a function? I don't believe modification is anything we will ever need!

[at-wat]

In stdlib, default things are often defined as global variable, e.g. net.DefaultResolver: https://golang.org/pkg/net/#pkg-variables

One of my intention of this was providing an example of ConnectContextMaker implementation on godoc.
I'll make it a function, unexpose, and explicitly add an example as a comment.

[Sean-Der]

Sorry I am wrong! Wasn't aware this was a Go pattern. I just always avoid globals and the possibility of people modifying things.

If you want to revert and go back to the way you had it I am happy with that :)

[at-wat]

updated

[at-wat]

But yea, it may allow something bad. If one of the library does

func init() {
    net.DefaultResolver = &net.Resolver{
        Dial: EvilDial,
    }
}

It's weird.

[daenney]

So I agree this is a pattern in stdlib, but I think this is a place where we should deviate from stdlib, and I'm happy to see the defaultConnectContextMaker instead.

I understand why it's historically shown up in stdlib, net/http does something similar with DefaultHTTPClient so you can call a naked http.$Method without needing to create your own http.Client first, but it's proven to be quite the footgun.

[Sean-Der]

With the next context API is it possible to be notified of a Close now without doing a Read? This is something WebRTC needs. I have a case where there is no SCTP traffic, so the DTLS alert is the fastest way to kill the connection. I was hoping I could do.

ctx, _ := context.WithCancel(context.Background())
c, err := dtls.Client(...)
if err != nil {
  return err
}

go func() {
   <-ctx.Done()
   if ctx.Err() != nil {
     fmt.Println("DTLS Session has ended in error!")
   }
}()

[Sean-Der]

@at-wat sorry for being so slow, left some questions! I am happy with this landing in master, we can always iterate/continue to work on it :)

[at-wat]

@Sean-Der unfortunately, #191 (comment) is not possible.
context.Context interface doesn't provide a way to cancel itself.
ctx, _ := context.WithCancel(context.Background()) is never been cancelled and just causes context leak.

I think it's better to add a state callback for this purpose.

[Sean-Der]

@at-wat I am happy with merging! We can always improve things as we go, such a shame about the leakage :(

I will add the state callback soon, need it for good WebRTC behavior

[at-wat]

Thank you for reviewing!