Prevent AddPeerInfo to override existing server information #1752
Conversation
Pull reviewers statsStats of the last 30 days for popstellar:
|
|
|
||
| _, ok := p.peersInfo[socketId] | ||
| if ok { | ||
| return xerrors.Errorf("peersInfo already contains [%s]", socketId) |
There was a problem hiding this comment.
I think the logs would be a bit more readable if they included the peer's info than the socket id. Maybe something like "peersInfo already contains socketid id for server peer's address would be useful for debugging.
There was a problem hiding this comment.
I changed it to also include the peer's info.
|
|
| _, ok := p.peersInfo[socketId] | ||
| if ok { | ||
| return xerrors.Errorf("peersInfo already contains socketId %s for server %s", socketId, info) | ||
| } |
There was a problem hiding this comment.
You should change info inside the xerrors.Errorf by the value you get from p.peersInfo[socketId] because we want to say that the socket is already used by an other server.
There was a problem hiding this comment.
Or something like 'peersInfo cannot add peer %s because socketId %s already exists'
There was a problem hiding this comment.
Do you thing that including both values of info could be useful ?
for example "peersInfo cannot add peer new_value because it already contains peer current_value for socketID id"
There was a problem hiding this comment.
Yes but in this case you should only print the peerId without the servers informations for both
There was a problem hiding this comment.
peersInfo cannot add new peer new_peerID because socket socketID is already used by peer current_peerID
There was a problem hiding this comment.
peersInfo is a map of socketId to ServerInfo, maybe the following could be more readable ?
"cannot add new_peer_info because peersInfo[id] already contains current_peer_info"
There was a problem hiding this comment.
I don't think that adding the peer information other than its public key is useful in this case.
|
|
|
|
|
Addresses the issue #1748
The
AddPeerInfofunction is now checking if it already contains a value for the specified socketId before writing the server information.If it is called with an existing socketId, it will throw an error.