p2p feature PR: CR fixes#6038
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## feature/p2p #6038 +/- ##
============================================
Coverage 56.11% 56.12%
============================================
Files 488 488
Lines 69419 69427 +8
============================================
+ Hits 38957 38965 +8
- Misses 27807 27814 +7
+ Partials 2655 2648 -7 ☔ View full report in Codecov by Sentry. |
| var host interface{} = req.Host | ||
| addrOrPeerID := req.Host | ||
| // p2p/http clients have per-connection transport and address info so use that | ||
| if len(req.Host) == 0 && req.URL != nil && len(req.URL.Host) == 0 { |
There was a problem hiding this comment.
Just thinking out loud — there's no way a weird HTTP request that didn't come through libp2p HTTP mode could force an invalid cast to (*peer.AddrInfo) below for a URL by requesting an empty host or something?
There was a problem hiding this comment.
good point, added a type check and return an error. That's why we practice CR :)
|
|
||
| // networkNames: lists the networks to which the given address belongs. | ||
| networkNames map[string]bool | ||
| mu *deadlock.RWMutex |
There was a problem hiding this comment.
a quick git grep deadlock.RWMutex shows that in the rest of the codebase we seem to always use deadlock.RWMutex as a value not a pointer.. but I guess that would require storing *addressData in the metadata instead of a value?
| mu *deadlock.RWMutex | |
| networkNamesMu deadlock.RWMutex |
There was a problem hiding this comment.
it have to be a pointer since there are type casts and copying
jasonpaulos
left a comment
jasonpaulos
left a comment
There was a problem hiding this comment.
Addressed my concerns, and the other changes look good as well (especially getting rid of the interface{} types)
3 participants
Summary
Addressed most code review items from #5939
Additionally fixed two data races:
Also fixed node.ledger descriptors leak that prevented
./nodetest -raceto pass due to extra connection opened by libp2pRemaining items:
EnableP2PHybridModevsEnableP2Pafter merging p2p: hybrid node net identity for connection deduplication #6035 (otherwise a merge conflict)