Buffer early tags in temporary entries

[raulk]

A patch I had cooked up a while ago and forgot to submit.

Occasionally protocols try to tag peers in the connection manager before the latter has learnt about the connection via the Connected callback.

This patch buffers such tags in temporary entries. Such entries are safeguarded during the grace period, and once this period has elapsed, they take precedence for pruning. Hence they don't build up over time and drift away automatically.

[vyzo]

shouldn't we do this in upsert too?

[raulk]

Ah yeah, too recent.

[raulk]

Fixed.

[vyzo]

Maybe it make sense to treat them as regular tags and not prune them.

[Stebalien]

Alternatively, we could change the semantics and say that all tags persist even after closing the connection for some grace period. garbage collecting tags for disconnected peers periodically. This will likely have little overhead and may make some things more predictable.

For example, I believe it's possible for the connection manager to see a peer disconnect while a different service doesn't (if we get a simultaneous disconnect+connect).

[raulk]

Hm, that would be valuable in an intermittent disconnection + reconnection. However, I think it's confusing because most protocols are designed to entirely drop internal state when a peer disconnects, and reconstruct it starting from scratch when it reconnects. Or do any protocols keep a "ghost" state? Honestly I think it could cause more problems than advantages.

[raulk]

In fact, we have nothing in the system that will try to re-establish a lost connection automatically. The reconnection is always triggered by a protocol.

[Stebalien]

My main concern is that not all services will necessarily see the disconnect. They'll see the connection drop but services usually only drop state when all connections drop. At the moment, this will leave untagged peers.

However, I agree that a grace period like that would be kind of weird. What if we never GC tags? That is, we independently track tags/connections and expect services to remove tags when they no longer need a peer. As far as I can tell, almost all of our services correctly do this anyways.

IMO, that's the least surprising way to handle this given that we assign weights to peers and not individual connections.

[Stebalien]

small nits you can ignore

[vyzo]

Do not merge yet, there is a race with decremented tags on upsert that can be triggered by relay code.
Specifically, we have goroutines waiting for stream closure to decrement tags for peers when the hop stream is closed in both directions.
If we happen to prune/disconnect a peer with a hop stream, then the decrement upsert will happen in a disconnected peer and result in a temporary tag with negative value.
The problem is compounded if the peer happens to reconnect before the grace period, as it will be permanently tagged negative.

Suggested change: Either don't create temporary tags on upsert or ignore tags with negative values.

[vyzo]

See #39 (comment)

[raulk]

@vyzo – based on the last comment, it seems the semantics of Upsert are unfit that particular case. We should perhaps rename this operation to Update with an upsert flag on the method signature that would be false in this case -- so as to only affect existing entries.

[Stebalien]

@vyzo bump.

[vyzo]

I don't like the idea of renaming the Upsert method, but I guess I can modify the call sites to return zero instead of decrementing to negative when the condition is triggered.
On the other hand we can add Update with the generalized signature alongside Upsert, and that could be used instead in the decrementing.

[raulk]

I propose to merge this as it respects the semantics of Upsert. We might want an Update method that strictly updates the peer if and only if a previous tag exists.

Discussed out of band and decided then problem is in the Upsert semantics.

[vyzo]

I have update the callsites to Upsert to not decrement a 0 tag, which resolves the pathology.
An Update method, companion to Upsert, sounds fine.

[vyzo]

this will need rebase; let me know if you think a review is needed post rebase.