Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

context.Context based cancellation #189

Closed
at-wat opened this issue Feb 10, 2020 · 9 comments · Fixed by #191
Closed

context.Context based cancellation #189

at-wat opened this issue Feb 10, 2020 · 9 comments · Fixed by #191
Labels

Comments

@at-wat
Copy link
Member

at-wat commented Feb 10, 2020

Proposed at #147.

#147 (comment)

@Sean-Der:
What do you think of updating Client/Server to take a context? I need a way to be notified that an error has occurred, and it looks like taking a Context is the proper way to have a async way to bubble up an error. I talk about it in #151 a bit.

@at-wat
Copy link
Member Author

at-wat commented Feb 10, 2020

It's also related to #163 (comment).

The new API could be:

func Dial(ctx context.Context, network string, raddr *net.UDPAddr, config *Config) (*Conn, error) {}

func Client(ctx context.Context, conn net.Conn, config *Config) (*Conn, error) {}

func Server(ctx context.Context, conn net.Conn, config *Config) (*Conn, error) {}

In this API, we might be able to drop ConnectTimeout option and ConnectTimeoutOption helper.

@at-wat at-wat added the v2 label Feb 10, 2020
@at-wat
Copy link
Member Author

at-wat commented Feb 10, 2020

Adding ctx context.Context makes the API different from crypto/tls.
We can also keep the current Dial/Client/Server and add DialWithContext/ClientWithContext/ServerWithContext.

@daenney
Copy link
Member

daenney commented Feb 10, 2020

I'd vote for the second option, adding the WithContext methods. There's a pretty decent precedent in net/http for doing that, and I'd prefer to not have our exported methods that share a name with crypto/tls diverge.

@daenney
Copy link
Member

daenney commented Feb 10, 2020

On net.Dialer though you can already pass in a context, so perhaps it's not needed?

@at-wat
Copy link
Member Author

at-wat commented Feb 10, 2020

Dialer sounds that is only for Dial, but we have to do the same things on Cilent and Server. And, most of the things in Dialer can be provided by Config.
So, I think Dialer is not very useful for us.

@at-wat
Copy link
Member Author

at-wat commented Feb 11, 2020

I opened a draft PR at #191.

One problem is that Listener.Accept internally calls Server, but Accept can't take context.Context as it waits until a new connection is requested from outside.

@at-wat
Copy link
Member Author

at-wat commented Feb 11, 2020

To be discussed

ConnectTimeout config 06468a3

Having timeout both by context.Context and dtls.Config is confusing.
However, as Accept should internally start connection timeout timer, it can't receive context.Context. In addition, listener is an implementation of net.Listener now, adding a new method makes it diverged from tls.Conn.

Error value on timeout dbf9263

Currently, errConnectTimeout is returned on timeout, but returning an error that Is(context.DeadlineExceeded) would be better.
ref: golang/go#31449

@at-wat
Copy link
Member Author

at-wat commented Feb 11, 2020

ConnectTimeout config

Adding a

	// function to make a context used in Dial(), Client(), Server(), and Accept().
	// If it is nil, default context with 30s timeout is used.
	ConnectContextMaker func() (context.Context, func())

to dtls.Config and doing

	ctxParent, cancelParent := context.WithCancel(context.Background())
	defer cancelParent()

	config := &dtls.Config{
		ConnectContextMaker: func() (context.Context, func()) {
			return context.WithTimeout(ctxParent, 30*time.Second)
		},
	}
	listener, err := dtls.Listen("udp", addr, config)
	conn, err := listener.Accept()
func (l *listener) Accept() (net.Conn, error) {
	c, err := l.parent.Accept()
	if err != nil {
		return nil, err
	}
	ctx, cancel := l.config.ConnectContextMaker()
	defer cancel()
	return ServerWithContext(ctx, c, l.config)
}

allows to Inherit the parent context to listener.Accept() internal context.
This should be helpful for servers usage to properly shutdown quickly.

(06468a3 implements this)

@at-wat
Copy link
Member Author

at-wat commented Feb 12, 2020

@daenney @Sean-Der how do you think these APIs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants