panic: putting same entry twice not supported

[AskAlexSharov]

[vyzo]

Ouch, seems our newly incorporated cache has a bug.

[Wondertan]

Our nodes are crashing because of this, and we need this fixed ASAP. If no one is working on this, we can take it,

[vyzo]

sure, go ahead.

Correct behaviour is to ignore the message.

[Wondertan]

The Has on cache is checked here with correct synchronization. However, false from Has can be returned even if the message already in the cache, because the message is outdated(time since first seen > TTL)

[Wondertan]

We can either:

• Simply don't panic and ignore the message
• Or sweep the message in place on Has when the time since first seen > TTL
  • This way panic will never be triggered

[Wondertan]

I am fine with both, but my intuition inclines toward the second option. WDYT, @vyzo

[vyzo]

just ignore the message i would say, panic is very bad.

[vyzo]

dont do the sweep in Has, will need xlock.

[vyzo]

Also, we need to implement background sweeping at some point, for both cache impls, as this eager sweep business has implications for locking.

[Wondertan]

There are many places where processLoop is locked unnecessarily :) Network.Connectedness can look it for a while

[Wondertan]

dont do the sweep in Has, will need xlock.

I know, but it seemed fine. Though background sweeping is a better approach so, let's just remove panic for now, as you've said

[vyzo]

yeah, although we try to make it fast.

But point taken, ok, lets fix Has.

But please do remove the panic, it needs to ignore the message (and maybe log something in debug).

[vyzo]

We are racing ;)

To summarize: Lets fix Has while at it. If you feel so inclined, feel free to do bg sweeping.

Other than that, the panic must go.

[vyzo]

More generally, if we are using bg sweeping, we can greatly simplify everything.

We dont need queues, just a map of mid to expiry. Has shouldnt even check time, just map presence. And bg sweeping just clears everything expired.

We can and should do for both implementations.

[Wondertan]

Ok, bg sweeping can be done in a separate PR. Let's remove the panic as a start: https://github.com/libp2p/go-libp2p-pubsub/pull/522.

[Wondertan]

@vyzo, mind releasing a patch?

[vyzo]

We certainly can, but i was thinking of doing bg sweeping first (unless you want to do it).

[Wondertan]

Fair, no need to keep that tech debt around.