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

canceling deadline context not honored #1399

Open
bhassink opened this issue Oct 27, 2022 · 1 comment
Open

canceling deadline context not honored #1399

bhassink opened this issue Oct 27, 2022 · 1 comment

Comments

@bhassink
Copy link

When providing a deadline context to Client.ExchangeContext(), canceling is only honored during the dial stage but not during the read or write stage.

@ThinkChaos
Copy link

The root cause for this is ExchangeWithConnContext using the given context's deadline (see client.go) but not the context itself.

Another issue this causes is ExchangeContext can return before the context is actually done because of the the various timer jitter. I ran into this when writing tests and asserting that ctx.Err() != nil, which wasn't the case because of jitter randomness and the underlying net.Conn timeout timer firing before the context's timer.
This specific issue could be fixed by blocking on <- ctx.Done() when a timeout occurs and the deadline used comes from the context but I'm not sure if that's worth it.

It also means that when a timeout occurs, errors.Is(err, ctx.Err()), errors.Is(err, context.Canceled) and errors.Is(err, context.DeadlineExceeded) are always false.

I think that unfortunately doing a proper fix for this would need a change to Go's standard library since we need net.Conn to support something like ReadContext.
Then ExchangeWithConnContext could create 2 child contexts of the user provided one: one for all read operations, and one for the writes. That way the user context, and the configured time outs are all respected.

A less ideal fix that might be acceptable is to wrap the IO in a goroutine that we leave running in the background if the context ends before the exchange completes:

func (c *Client) ExchangeWithConnContext(ctx context.Context, m *Msg, co *Conn) (r *Msg, rtt time.Duration, err error) {
	type result struct {
		r   *Msg
		rtt time.Duration
		err error
	}

	ch := make(chan result, 1) // 1: prevent the sender from blocking forever ctx ended first
	go func() {
		defer close(ch)

		r, rtt, err := c.exchangeWithConnContext(ctx, m, co) // this would be the existing ExchangeWithConnContext
		ch <- result{r, rtt, err}
	}()

	select {
	case res := <-ch:
		return res.r, res.rtt, res.err

	case <-ctx.Done():
		return nil, 0, ctx.Err()
	}
}

The good news is this can be done as a workaround outside of this module :)
As writen, it doesn't fix ctx.Err() possibly being nil but that could be added.

@miekg would you be open to merging such a fix?

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

No branches or pull requests

2 participants