Multiple PeerConnections with the same nodeId but different PeerAddress #1422
Conversation
vivien-apple
requested review from
andy31415,
anush-apple,
BroderickCarlin,
chrisdecenzo,
hawk248,
jelderton,
robszewczyk,
saurabhst and
woody-apple
as code owners
src/transport/SecureSessionMgr.cpp
Outdated
| // connection, mark the previous connection invalid in order to not have duplicate node ids. | ||
| if (connection->mPeerConnections.FindPeerConnectionState(header.GetSourceNodeId().Value(), &state)) | ||
| { | ||
| connection->mPeerConnections.MarkConnectionInvalid(state); |
There was a problem hiding this comment.
We may want a callback when this happens. For any persistent channels (TCP or BLE) we will need to close the connection.
There was a problem hiding this comment.
Any objections if I change the name of the current callbacks ?
I was thinking of something like:
OnMessageReceived -> OnMessage
OnReceivedError -> OnError
It would let me reuse the OnError callback and the embedder can then decide what to do depending on the type of errors.
There was a problem hiding this comment.
Seems reasonable. Can we somehoe give the OnError callback context to detect when a 'overwrite/cancel' is provided?
Maybe OnExpiry then is also OnError.
There was a problem hiding this comment.
@woody-apple Yeah sorry for the latency I'm trying to figure out what I want exactly.
@andy31415 OnExpiry ? Does it exists in the tree or did you mean something else ?
There was a problem hiding this comment.
Connection expiry exists currently in the tree and is treated reasonably stand-alone:
we may need some thought since this particular type of error has sideeffects (need to free up resources on the connection)... but maybe any error on a connection should close/free it.
There was a problem hiding this comment.
I also see we are piling up changes and wondering if we should try to make some changes here or file an issue and work on these things separately.
There was a problem hiding this comment.
Right. So I end up just calling the expiry callback when this thing happens. Would be better to do it separately.
vivien-apple
force-pushed
the
Various_SecureSessionMultipleNodeIdsDifferentPeerAddress
branch
from
57c80b7 to
41eec3e
Compare
|
|
Problem
The second node can also forced the connection to the first node to stay active.
I was unsure of what is the expected behavior here, so I made a tentative patch that consider the last connection to be the right one.