mu.lock: after acquiring m.ch, why does the re-check only look at closed and not ctx?

[dangquyitt]

Hi, I was looking at mu.lock and had a question about this part:

func (m *mu) lock(ctx context.Context) error {
	select {
	case <-m.c.closed:
		return net.ErrClosed
	case <-ctx.Done():
		return fmt.Errorf("failed to acquire lock: %w", ctx.Err())
	case m.ch <- struct{}{}:
		// To make sure the connection is certainly alive.
		// As it's possible the send on m.ch was selected
		// over the receive on closed.
		select {
		case <-m.c.closed:
			// Make sure to release.
			m.unlock()
			return net.ErrClosed
		default:
		}
		return nil
	}
}

After acquiring the lock, closed is checked again in case both branches were ready.

Why isn't ctx.Done() checked here as well?

Could ctx be canceled right after m.ch is selected, causing lock to return nil while holding the lock with an already-canceled context?

[mafredri]

Hey @dangquyitt. Good observation and valid question.

The reason we re-check closed is to uphold the contract that a successful lock means the connection is live. Because Go randomly selects the channel if multiple are ready, simply grabbing the lock doesn't ensure the connection isn't also closed.

Could ctx be canceled right after m.ch is selected, causing lock to return nil while holding the lock with an already-canceled context?

Yes, but it's not a problem (at least currently). It's not as important because ctx is caller-provided and indicates the how long they're willing to wait. Granted, select will be random here as well and if a caller repeatedly calls lock and unlock without ever checking their ctx in-between, they could theoretically be going forever if unlucky.

These re-checks are racy, however, and there's nothing that says the state won't change between check and return. The simplest solution is probably just to check if ctx.Err() != nil before the first select so that we've exhausted the possibility.

[dangquyitt]

Thanks for the explanation, @mafredri.

I think my only remaining question is about error priority when both states are already observable before the main select.

If we check ctx.Err() first:

if err := ctx.Err(); err != nil {
	return err
}

then the context error always wins.

If we want net.ErrClosed to win instead, we could check closed first:

select {
case <-m.c.closed:
	return net.ErrClosed
default:
}

This still wouldn't fully remove the race, but at least it would make the behavior clear for states that are already observable before entering the main select.

Do you think the library should define this priority explicitly, or is either error acceptable here?