feat(connecteventmanager): block Connected() until accepted

[rvagg]

Fixes: #432

Minimal attempt at solving #432

[welcome]

Thank you for submitting this PR!
A maintainer will be here shortly to review it.
We are super grateful, but we are also overloaded! Help us by making sure that:

• The context for this PR is clear, with relevant discussion, decisions
  and stakeholders linked/mentioned.

• Your contribution itself is clear (code comments, self-review for the
  rest) and in its best form. Follow the code contribution
  guidelines
  if they apply.

Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
Next steps:

• A maintainer will triage and assign priority to this PR, commenting on
  any missing things and potentially assigning a reviewer for high
  priority items.

• The PR gets reviews, discussed and approvals as needed.

• The PR is merged by maintainers when it has been approved and comments addressed.

We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution.
We are very grateful for your contribution!

[codecov]

Codecov Report

Merging #435 (22fd90d) into main (dd32d67) will increase coverage by 0.22%.
The diff coverage is 51.02%.

@@            Coverage Diff             @@
##             main     #435      +/-   ##
==========================================
+ Coverage   49.80%   50.02%   +0.22%     
==========================================
  Files         249      249              
  Lines       29972    29990      +18     
==========================================
+ Hits        14928    15003      +75     
+ Misses      13615    13554      -61     
- Partials     1429     1433       +4     

Files Changed	Coverage Δ
bitswap/network/connecteventmanager.go	50.99% <51.02%> (+50.99%)	⬆️

... and 7 files with indirect coverage changes

[rvagg]

I'd like some help sanity checking this because I'm not entirely clear of the atomicity of some of these operations.

We rely on connectEventManager's mutex to do most things, but there is a call to waitAccept that is unsynchronised, it just does a <-p.accepted; that could be happening while we're doing this operation of setting accepted to a new channel—although we're only doing this if the channel is already closed so <-p.accepted should have been a quick noop; is that atomic though? Is there potential for a conflict between <-p.accepted where p.accepted is closed, and p.accepted = make(chan struct{})?

[rvagg]

All good with our test suite that makes heavy use of bitswap and has been heavily flaky because of this problem: filecoin-project/lassie#383

[hannahhoward]

I agree no per peer mutex since everything is already mutexed.

In your new simpler implementation, there is an edge case I'm not sure whether you should care about:

• setState called, isPending set true, change created
• before change processed, setState called again (to a same or different new state)
  • since isPending = true, no new change encoded, and waitNoop returned. however, the change has yet to be handled

Truthfully, the edge case where this causes downstream problems feels possibly non-existant. Just want to identify it.

The potential solution is to put handled back on the peerState, and return a func() that uses it whenever isPending = true -- but if you do so, I recommend you copy the reference out of the peerState before returning the wait func, so that any mutations to the channel in the peer state don't effect the callback. This can be done with a simple closure:

func makeWaitFunc(handled chan struct{}) waitFn {
   return func() {
      <-handled
   }
}

Also, are there any edge cases where handled never gets closed()? Like the queue shuts down for example?

[rvagg]

Good feedback!

Here's what I've done:

1. Handled the Stop() end condition by watching the c.done channel as well:

func (c *connectEventManager) makeWaitFunc(handled chan struct{}) waitFn {
	return func() {
		select {
		case <-handled:
		case <-c.done:
		}
	}
}

2. Handled the pending case within the current structure with this branch that piggy-backs the existing change in the queue:

	} else if state.pending {
		// Find the change in the queue and return a wait function for it
		for _, change := range c.changeQueue {
			if change.pid == p {
				return c.makeWaitFunc(change.handled)
			}
		}
		log.Error("a peer was marked as change pending but not found in the change queue")
	}

[hannahhoward]

LGTM by me.

[rvagg]

Added a simple test to lock this behaviour in - Connected() blocks until PeerConnected() returns.

[Jorropo]

The connectEventManager is a very complex piece of code for what it does and it took tens of minutes to properly understand it properly.
It's role is to maintain an atomic state of connected / not connected and dispatch callbacks on state transitions. I've submitted #436 which does this using a map. Could any of you please take a look @rvagg or @hannahhoward please ?

[rvagg]

To reiterate the discussion on Slack - I'm not opposed to your approach in #436, but ripping out the existing piece of infrastructure is more radical than attempting to fix it; at least for the purposes we're trying to achieve right now. Perhaps this could be a two-step thing. I see this as a Chesterton's Fence situation - we're all acknowledging that we don't fully appreciate why the connecteventmanager exists and what it's aiming to achieve, and in that case the more prudent approach, rather than just ripping it out, might be to approach it more incrementally ("prudence is to understand why the fence is there in the first place before you attempt to take it down").

There's a singular issue we're trying to fix, and that is the race condition that exists between when a client is Connected and when the client is notified that PeerConnected so it can perform proper per-client initialisation.

Ditching connecteventmanager possibly fixes that, and other problems that relate to it; but I wouldn't mind having a commit in the main branch that solves the one thing we're concerned about now that we can point to, even if later commits do more radical things; so we can choose to be incremental and opt-in to being more radical in our downstream use of this code.

[Jorropo]

SGTM, this is tricky code and I'm not sure about all the interactions but tests are passing.
Should get #436 merged within a month ideally.