-
Notifications
You must be signed in to change notification settings - Fork 60
Conversation
findPeer needs to be updated to support an abort controller
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's great to have this really comprehensive testing for random walk 👍
I'm not sure I'd include the AbortController logic until we move to an async/await API
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 am not against adding the AbortController logic in this PR. But we should have a comment explaining it will not be used for now / WIP
test: disable random walk by default for TestDHT util
I've removed the runningHandler logic and simplified the code. I also bumped up the default timeout to 30s from 10s. Since we are passing on the timeout to findPeer, and it actually times the query out, giving it more time to run will improve the effectiveness. I also disabled random walk in the TestDHT util by default to keep the tests isolated better. We're managing connects as needed in the tests, having random walk running will just cause more test chaos. I added a comment to the |
@@ -73,7 +73,7 @@ function connect (a, b, callback) { | |||
|
|||
function bootstrap (dhts) { | |||
dhts.forEach((dht) => { | |||
dht.randomWalk._walk(1, 1000, () => {}) | |||
dht.randomWalk._walk(1, 10000, () => {}) |
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.
Now that timeout is actually being passed to findPeer
I bumped this to 10seconds so the random walk doesn't get killed after 1s. This was causing test failures because routing tables weren't doing enough connecting when it was depending on the walk.
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.
LGTM
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.
One comment but otherwise LGTM 👍
Thanks for cleaning up how the tests work as well!
Random Walk had some bugs, this fixes them:
findPeer
wasn't given the walks timeout. So if you specified 10seconds as your walk timeout, thefindPeer
logic would keep running until its default timeout kicked in (60 seconds)I added a suite of tests just for RandomWalk to test the failures I was seeing.
This also adds support for AbortController in random walk and passes it to findPeer, however, findPeer currently isn't listening to the abort signal so it won't actually stop the query. My thought is to wait until we finish #82 and then add the abort logic to all of the query methods. Once we do, random walk will immediately benefit because it already passes the signal in. @dirkmc @vasco-santos let me know what you think, and I can update accordingly.