[client] lookup for management domains using an additional timeout#4983
[client] lookup for management domains using an additional timeout#4983
Conversation
in some cases iOS and macOS may be locked when looking for management domains during network changes This change introduce an additional timeout on top of the context call
WalkthroughAdds an async DNS lookup helper with extended timeout and an internal result type; replaces direct resolver calls with the helper in AddDomain, adding logging, timeout warning, context cancellation handling, and error propagation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
client/internal/dns/mgmt/mgmt.go (1)
168-196: Consider minor naming and logging refinements.The async lookup implementation is correct and the buffered channel ensures no goroutine leaks. A few optional suggestions:
Channel naming:
errChancarries both IPs and error;respChanwould be more accurate.Logging level:
Infolevel for every lookup start/done may be verbose in production. ConsiderDebugfor the routine messages, keepingWarnfor the timeout case.🔎 Proposed refinements
func lookupIPWithExtraTimeout(ctx context.Context, d domain.Domain) ([]netip.Addr, error) { - log.Infof("looking up IP for mgmt domain=%s", d.SafeString()) - defer log.Infof("done looking up IP for mgmt domain=%s", d.SafeString()) - errChan := make(chan *ipsResponse, 1) + log.Debugf("looking up IP for mgmt domain=%s", d.SafeString()) + defer log.Debugf("done looking up IP for mgmt domain=%s", d.SafeString()) + respChan := make(chan *ipsResponse, 1) go func() { ips, err := net.DefaultResolver.LookupNetIP(ctx, "ip", d.PunycodeString()) - errChan <- &ipsResponse{ + respChan <- &ipsResponse{ err: err, ips: ips, } }() var resp *ipsResponse select { case <-time.After(dnsTimeout + time.Millisecond*500): log.Warnf("timed out waiting for IP for mgmt domain=%s", d.SafeString()) return nil, fmt.Errorf("timed out waiting for ips to be available for domain %s", d.SafeString()) case <-ctx.Done(): return nil, ctx.Err() - case resp = <-errChan: + case resp = <-respChan: }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
client/internal/dns/mgmt/mgmt.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: release_ui
- GitHub Check: iOS / Build
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: JS / Lint
- GitHub Check: Windows
🔇 Additional comments (2)
client/internal/dns/mgmt/mgmt.go (2)
30-34: LGTM!The struct appropriately encapsulates both the result and error for async channel communication.
108-111: LGTM!The integration with the new helper function is clean and preserves the existing error-handling behavior.
|
2 participants
Describe your changes
in some cases iOS and macOS may be locked when looking for management domains during network changes
This change introduce an additional timeout on top of the context call
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
Bug Fixes
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.