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

forget DHT pubkey of offline friend after DHT timeout #615

Merged
merged 1 commit into from Nov 25, 2017

Conversation

@zugz

zugz commented Nov 19, 2017

According to my tests, this fixes #240. I expect it also fixes #237.

The problem was as follows: if a friend goes offline for long enough, we stop
searching for them on the DHT, but then if they come back with the same DHT key
(as will be the case if the network connection was interrupted but their client
wasn't restarted), prior to this patch we didn't initiate a new search.

The reason that it took two disconnections for this to cause a problem is that
we keep information on their ip address and tcp relays, and attempt to reuse
after a disconnect if there's still a DHT search ongoing. So if the first
disconnect lasted for at least 122s, the DHT search was deleted, but we could
still connect back using the old details once they reconnected; but then on a
subsequent disconnect, the old details wouldn't be used and nor would a DHT
search start up, so we'd never reconnect to the friend.

This PR zeroes out the friend's DHT PK when we delete them from the DHT, so the
next dhtpk packet we get from them will trigger us to add them back to the DHT
search list.


This change is Reviewable

@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Nov 19, 2017

Member

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Member

robinlinden commented Nov 19, 2017

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@robinlinden robinlinden added this to the v0.1.11 milestone Nov 19, 2017

@SkyzohKey SkyzohKey added PR:ready and removed PR:review_needed labels Nov 19, 2017

@Diadlo

This comment has been minimized.

Show comment
Hide comment
@Diadlo

Diadlo Nov 20, 2017

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

Diadlo commented Nov 20, 2017

:lgtm_strong:


Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@SkyzohKey

This comment has been minimized.

Show comment
Hide comment
@SkyzohKey

SkyzohKey Nov 21, 2017

@zugz Can you please sign the CLA for this to be mergeable ?

SkyzohKey commented Nov 21, 2017

@zugz Can you please sign the CLA for this to be mergeable ?

@zugz

This comment has been minimized.

Show comment
Hide comment
@zugz

zugz Nov 21, 2017

zugz commented Nov 21, 2017

@toxtox

This comment has been minimized.

Show comment
Hide comment
@toxtox

toxtox Nov 21, 2017

@zugz : Thanks for analyzing, describing and fixing (hopefully) the related bugs with your PR. This would be a milestone for TOX.

toxtox commented Nov 21, 2017

@zugz : Thanks for analyzing, describing and fixing (hopefully) the related bugs with your PR. This would be a milestone for TOX.

@zugz

This comment has been minimized.

Show comment
Hide comment
@zugz

zugz Nov 21, 2017

zugz commented Nov 21, 2017

@toxtox

This comment has been minimized.

Show comment
Hide comment
@toxtox

toxtox Nov 21, 2017

@zugz

Could you test to see if this does fix it?

Oh, this would be a challenge for me. I guess I have to compile the c-toxcore library with TRifA or Antox but I have no cross-compile tool-chain until now.
I hope that @zoff99 or @wiiaam could help with providing a development binary of TRifA or Antox including this c-toxcore-PR.
Then I will immediately test it.

toxtox commented Nov 21, 2017

@zugz

Could you test to see if this does fix it?

Oh, this would be a challenge for me. I guess I have to compile the c-toxcore library with TRifA or Antox but I have no cross-compile tool-chain until now.
I hope that @zoff99 or @wiiaam could help with providing a development binary of TRifA or Antox including this c-toxcore-PR.
Then I will immediately test it.

@zugz

This comment has been minimized.

Show comment
Hide comment
@zugz

zugz Nov 21, 2017

zugz commented Nov 21, 2017

@sudden6

This comment has been minimized.

Show comment
Hide comment
@sudden6

sudden6 Nov 21, 2017

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

sudden6 commented Nov 21, 2017

:lgtm_strong:


Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@SkyzohKey SkyzohKey added this to Triage in Mobile optimizations via automation Nov 24, 2017

@robinlinden robinlinden merged commit 0fce3fc into TokTok:master Nov 25, 2017

3 of 5 checks passed

ci/circleci CircleCI is running your tests
Details
license/cla Contributor License Agreement is not signed yet.
Details
code-review/reviewable 3/1 LGTMs
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Mobile optimizations automation moved this from Triage to Done Nov 25, 2017

@robinlinden

This comment has been minimized.

Show comment
Hide comment
@robinlinden

robinlinden Nov 25, 2017

Member

Awesome work fixing this long-standing bug, @zugz! We're lucky to have you involved. :)

Member

robinlinden commented Nov 25, 2017

Awesome work fixing this long-standing bug, @zugz! We're lucky to have you involved. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment