dtChannel.close appears to have a lock condition in it

[hannahhoward]

While working on transport code, I noticed the code for dtchannel.Close() appears to have a potential lock condition.

func (c *dtChannel) close(ctx context.Context) error {
	var errch chan error
	c.lk.Lock()
	{
		// Check if the channel was already cancelled
		if c.requestID != nil {
			errch = c.cancel(ctx)
		}
	}
	c.lk.Unlock()

	// Wait for the cancel message to complete
	select {
	case err := <-errch:
		return err
	case <-ctx.Done():
		return ctx.Err()
	}
}

If requestID==nil, as I read this code, then errch is left nil, and the select will not return until the context completes, which seems off. I would think if requestid == nil, the channel is already closed, and we can return immediately. Plus if for some reason context never closes, this routine would never return.

Am I wrong? Am I reading this wrong.

[rvagg]

yeah, looks very dodgy but maybe depends on a bunch of contextual things:

• What are the cases where requestID could be nil here? Can we even have a case where it's nil and close() gets called? Maybe that's why this hasn't caused a problem.
• Does ctx get used for channels? Maybe we close contexts elsewhere, so the Done() gets triggered here?