More extractions from the Protocol struct#2641
Conversation
tomaka
force-pushed
the
more-protocol-extract
branch
from
a181d45 to
597d45c
Compare
| } | ||
|
|
||
| match protocol.poll(&mut Ctxt(&mut network_service.lock(), &peerset)) { | ||
| while let Ok(Async::Ready(_)) = connected_peers_interval.poll() { |
There was a problem hiding this comment.
As noted with the updating of is_offline and is_major_syncing, peers was previously only updated when something changed, now it will be updated every interval, whether something changed or not.
Unlike these other two fields, which are updated at each poll of the future returned by run_thread(I think), the interval could actually introduce some delays in the updating in some cases. I think this potentially could introduce problems, since whoever uses peers could still see peers which would have already been removed from ContextData. This could be made worse if anything ends-up blocking the networking runtime, since the interval could turn out to be longer than intended.
So if you must move this logic out of protocol, perhaps also for now update peers at each turn of run_thread?
There was a problem hiding this comment.
Ah yeah, I went for this because peers() is only ever used to answer an RPC query for diagnostic purposes, where it really doesn't matter if you're 500ms late.
It's probably worth adding a comment about that, though.
There was a problem hiding this comment.
In the last commit I also renamed peers() to peers_debug_info() in order to make it clearer.
gterzian
left a comment
gterzian
left a comment
There was a problem hiding this comment.
Looks like the tests just need an update in the light of the last commit...
|
Are the changes to the wasm lock files intentional? |
|
Running the WASM build scripts updates the Cargo.locks, but since I guess it's out of scope, I reverted that. |
|
Failures on CI appear unrelated, with the tests fixed(and 2 existing approvals), I think it's ready to merge... |
* Pass the TransactionPool explicitly * Extract finality_proof_provider * Remove Protocol::connected_peers * Add note and rename function * Fix tests * More test fixing * Revert the WASM locks, I guess * Add space * Remove space
3 participants
Removes the
transaction_pool,finality_proof_providerandconnected_peersfields fromProtocol.Same goal as usual: trying to remove everything that's not fully deterministic from
Protocol.