go-libp2p-pubsub-router icon indicating copy to clipboard operation
go-libp2p-pubsub-router copied to clipboard

go-ipfs#8586 added EOL for subscriptions

Open TobiaszCudnik opened this issue 3 years ago • 3 comments

Minimal (and probably naive) implementation of https://github.com/ipfs/go-ipfs/issues/8586.

Changes

  • EOL handled in pubsub directly because of republish on startup
  • GC for topics in pubsub router
    • EOL channel for each subscription
    • bump the delay after repeated resolve/publish requests
  • added key formatting for logs
  • assumptions
    • lifetime of IPNS in DHT separate from lifetime in pubsub
    • DefaultSubscriptionLifetime (pubsub) higher than DefaultRecordEOL (namesys)

Logging

export GOLOG_LOG_LEVEL="warning,pubsub-valuestore=debug,net:pubsub=debug,ipns-repub=debug,ipns=debug,namesys=debug"

TODO

  • tests
  • config Ipns.ReproviderDuration

While I havent looked at the tests just yet, I had hard time figuring out how to pass the config from go-ipfs to go-libp2p-pubsub-router through go-namesys.

TobiaszCudnik avatar Jan 12 '22 12:01 TobiaszCudnik

This needs some review (and perhaps further discussion in the linked IPFS issue). Having self-denotating topics (i.e. closing after 36hrs) is likely not the way to go here for GC.

@lidel or I should take a look or respond when some time frees up.

aschmahmann avatar Jan 14 '22 16:01 aschmahmann

Thanks for the review guys!

Having self-denotating topics (i.e. closing after 36hrs) is likely not the way to go here for GC.

The strategy is to re-publishing IPNS entries every DefaultRebroadcastInterval (in go-namesys/republisher/repub.go::Run), which will bump the subscriptions's EOL (in go-libp2p-pubsub-router/pubsub.go::Subscribe). Without being bumped the subscription will GC itself. This is tighter than manually unsubscribing.

[removed]

So the idea is to have expiration logic engage only if key is from ipns namespace.

Thats a valid point, I will pass the namespace down from go-ipfs to go-libp2p-pubsub-router, as suggested.

Edit: removed wrong info

TobiaszCudnik avatar Jan 20 '22 16:01 TobiaszCudnik

Ignore the part about publish / resolve above, Ive been debugging for too long and got it mixed up. The GC actually works well.

Changes:

  • TTL per namespace
  • tests
  • config entry Ipns.ReproviderDuration

Sibling PRs:

Ive used 1y for the default TTL to simplify the code and avoid nil pointers. Not sure if thats ok.

TobiaszCudnik avatar Jan 22 '22 20:01 TobiaszCudnik