-
Notifications
You must be signed in to change notification settings - Fork 579
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Only store peers in CONNECTED or CONNECTING states #2275
Conversation
d3cea35
to
10dbb97
Compare
@@ -424,19 +397,6 @@ export class Peer { | |||
|
|||
this._address = address | |||
this._port = port | |||
|
|||
if (address === null && port === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like we've removed this logic but we aren't calling PeerManager.getConnectionRetry.neverRetryConnecting
in place of removing this logic. Is that what we want to do? We call setWebSocketAddress
in a few places where I don't think this logic has been replaced
existingConnection.close(new NetworkError(error)) | ||
} | ||
} | ||
|
||
/** | ||
* Sets a WebSocket connection on the peer, moving it into the CONNECTING state if necessary. | ||
* Ignores the connection if the peer already has a WebSocket connection. | ||
* @param connection The WebSocket connection to set | ||
*/ | ||
setWebSocketConnection(connection: WebSocketConnection): void { | ||
if (this.state.type !== 'DISCONNECTED' && this.state.connections.webSocket) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
log line updated
Summary
Opening this as a draft to give some visibility into the work here.
TODO
Cap number of peerCandidates in the peerCandidateMap?Remove tryDispose functionTesting Plan
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.