-
Notifications
You must be signed in to change notification settings - Fork 42
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
fix: peer renewal connection drop & stream management #2145
Conversation
size-limit report 📦
|
I tried this with Dogfooding, Let's supersede this with #2137, where I am experimenting with no disconnections, but simply removing (temporarily) the peer from the protocol's management |
that's true, dfdea84 addressed issue only partially this 00c3261 is more comprehensive fix to another side of the problem please, try and tell me if you experience any, so far for me after and hour of running it works fine |
|
||
this.streamPool.delete(peerIdStr); | ||
this.prepareStream(peer); | ||
const stream = this.getStreamForCodec(peer.id); |
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.
if stream = await connection.newStream(this.multicodec);
in createStream
succeeds - then it will be attached to connection
and hence this.getStreamForCodec
will return it.
This is a bit convoluted at first but:
- we need to keep our single source of truth -
connection
object; - we need to have a lock so that multiple events schedule only one redundant stream creation;
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.
Looks good!
Do you think we should add tests that cover the uncaught functionality solved with this PR?
@@ -291,7 +291,8 @@ describe("Waku Filter V2: Subscribe: Single Service Node", function () { | |||
messageCollector.verifyReceivedMessage(index, { | |||
expectedContentTopic: topic, | |||
expectedMessageText: `Message for Topic ${index + 1}`, | |||
expectedPubsubTopic: TestPubsubTopic | |||
expectedPubsubTopic: TestPubsubTopic, | |||
checkTimestamp: false |
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.
@waku-org/js-waku I temporarily suppressed this check as it doesn't regress functional testability here
will re-enable it once I root cause it
Problem
Way too many
No stream available
issues.Solution
Seems there are multiple issues causing the problem:
When doing LightPush sometimes we do a
send
to peer connection to which was dropped.Fix: Drop connection only if peer was renewed, if wasn't - do not change list of peers on a protocol.
We create and keep way to many streams for protocols per peer.
Fix: Improve stream management and add locks.
Currently typical connection has way to many redundant streams.
After changes to `StreamManager` in this PR streams are better organized with only one redundant stream.
Notes