Conversation
initial implementation extracted from ipfs/kubo#10883
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #997 +/- ##
==========================================
- Coverage 61.56% 60.46% -1.10%
==========================================
Files 257 266 +9
Lines 32161 33257 +1096
==========================================
+ Hits 19799 20109 +310
- Misses 10751 11483 +732
- Partials 1611 1665 +54
... and 8 files with indirect coverage changes 🚀 New features to boost your workflow:
|
depends on ipfs/boxo#997
| func WithRefreshInterval(interval time.Duration) Option { | ||
| return func(c *Client) error { | ||
| if interval <= 0 { | ||
| return fmt.Errorf("refresh interval must be positive") | ||
| } | ||
| c.refreshInterval = interval | ||
| return nil | ||
| } | ||
| } |
There was a problem hiding this comment.
It would be good to introduce a minimum refresh interval that is hardcoded. We don't want people setting their refresh interval to 1s by mistake.
There was a problem hiding this comment.
Is tricky because we have tests that depend on very low interval to confirm things work.
What is more idiomatic, use testing.Testing() to detect test mode and lift min interval limit, or have explicit WithTestMode()?
hsanjuan
left a comment
hsanjuan
left a comment
There was a problem hiding this comment.
Thank you for writing such good code!
| // Stop gracefully stops the background updater if it's running. | ||
| // This is an alternative to cancelling the context passed to Start(). | ||
| // It's safe to call Stop() multiple times. | ||
| func (c *Client) Stop() { | ||
| c.updaterMu.Lock() | ||
| defer c.updaterMu.Unlock() | ||
|
|
||
| if c.updater != nil { | ||
| c.updater.Stop() | ||
| c.updater = nil | ||
| log.Infof("Stopped autoconf background updater") | ||
| } | ||
| } |
There was a problem hiding this comment.
If we can control lifecycle via context, perhaps we can avoid having a second way?
There was a problem hiding this comment.
I prefer to avoid lifecycle contexts if at all possible, and if they are the best solution, then do not make the visible outside the package and the context is not kept in the type. In other words, only keep the cancel function as a data member in the type and pass the context into a goroutine started when the type instance is created.
requests are made infrequently (24h default), no benefit from connection pooling
- make BackgroundUpdater type private (backgroundUpdater) - move updater callbacks to Client options (WithOnNewVersion, WithOnRefresh, WithOnRefreshError) - fix race conditions with proper mutex protection - handle zero timeout edge case in Start() - simplify API by hiding updater as implementation detail
avoids unnecessary config resolution calls when node is well-connected
3 participants
This PR adds
boxo/autoconfigclient library, cleaned up and extracted from ipfs/kubo#10883Used in: