-
Notifications
You must be signed in to change notification settings - Fork 21.6k
swarm/network/stream: RPC to request subscription info #18284
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
Conversation
ffca572 to
c41721d
Compare
nolash
left a comment
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.
Reviewed with the disclaimer that I know quite little about the streamer code in general
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.
Why delay and healthy-check?
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.
The health check is about the kademlia's health. It will be commented out/removed for now until we have it fixed.
The syncUpdateDelay is a streamer's internal delay time after which it starts actual syncing.
It is relevant to this test in so much as we need to wait at least this delay + the syncing duration time before we actually can check anything.
and
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.
Can't this also be done with messageevents?
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.
What do you mean?
There is no streamingStarted event or something like that, and the SyncUpdateDelay is part of the main code, so we need to set it to something and wait for it.
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.
Like also mentioned herer #18284 (comment) what I mean is then the streaming starts that streaming itself is actually devp2p messages being exchanged, no? Those messages could be inspected, I think;
- is it a syncSomethingType
- is it from a peer that hasn't sent this type of message yet
Not sure if I'm making myself clear...
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.
Yes I already updated the PR to wait for SubscribeMsg
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.
@nolash can you pls mark this as resolved if you agree or comment further if you think it still needs attention. Thanks
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.
The syncUpdateDelay is a streamer's internal delay time after which it starts actual syncing.
In such cases we should always get the value directly from the source where it's defined IMO.
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.
@nolash in this case no. If no SyncUpdateDelay is provided, the streamer will set it to 15 * time.Second, which is obviously too long for a test.
Thus we need to set it, which is why it is set to 500 * time.Millisecond for this test.
Check this #18284 (comment) comment please, that has the relevant snippet
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.
The syncUpdateDelay is a streamer's internal delay time after which it starts actual syncing.
In such cases we should always get the value directly from the source where it's defined IMO.
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.
Ohh, dangerous with the literal 4.
In fact, if this is the only way to get a message type then the simulation framework has skipped the functionality enabled in the p2p/protocols layer where you can get the message code from the struct type.
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.
I changed it to get the code via spec.
As a side note though, our tests are littered with tests using hardcoded message codes (see this very same test file, all other codes are literals).
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.
Let's make an issue for this!
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.
catch the context timeout in a select here?
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.
Actually, are we absolutely sure here that the message is not only sent but the change also processed at this point, resulting in the change in the streamer state?
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.
At this point we are sure that the message has been received. There is no way currently to make sure that a message has been processed - I dare to say that none of our tests based on listening to message exchange can actually do that. To that end we would need a node-wide event propagation system.
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.
yes that or message acks after state change :)
2ded8ce to
577f8b7
Compare
|
Reopened upstream as #18338 |
This PR adds to the debugging information available.
Over RPC, this code allows to request the
streamsyncing subscriptions and returns them as a map.It adds a simulation test which over a snapshot requests all subscriptions.
If provided with a
-printstats trueflag, it will print the results toSTDOUT