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

dht: consider IPv4 or IPv6 disconnected on operation done #73

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@sim590
Contributor

sim590 commented Jun 1, 2016

Put and Get wrapped done callbacks need to consider the case only ipv4 or ipv6 connection is available. In that case, don't block waiting for the disconnected network and call done callbacks if the operation succeeded on one of the networks.

This bug was introduced by both d44c590 ('put' donecb wouldn't be called) and bd08506 ('get' donecb wouldn't be called)

@sim590 sim590 added the bug label Jun 1, 2016

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 1, 2016

Contributor

[1] Let table be either the ipv4 or ipv6 table such that table is empty. Announce and Get structures won't be cleared until new nodes are added to the table. Also, done callbacks associated to the table won't even be called with failure status. In fact, after some attempts in dhtnode with ipv6 table empty:

>> ls
Searches:
s:synched, u:updated, a:announced, c:candidate, f:cur req, x:expired, *:known

Search IPv4 99757828bd57b564759d10e1d85cb9dc34179b6d gets: 0, age: 181 s [done] [synced]
 Common bits    InfoHash                       Conn. Get   Ops  IP
  1 e2216ad245ffb28bf7da1ec38f309c3cc1ecd8d9 * [  ] [u ] 192.168.1.100:5000

Search IPv4 9bb095aebe3939cf322054c0f0048863ab073b17 gets: 0, age: 86 s [done] [synced]
 Common bits    InfoHash                       Conn. Get   Ops  IP
  1 e2216ad245ffb28bf7da1ec38f309c3cc1ecd8d9 * [  ] [u ] 192.168.1.100:5000

Search IPv6 9bb095aebe3939cf322054c0f0048863ab073b17 gets: 5, age: -9223156897 s [not synced]
Queries:
Query[SELECT * ]
Query[SELECT * ]
Query[SELECT *  WHERE id=5]
Query[SELECT *  WHERE id=17710297678896355269]
Query[SELECT *  WHERE user_type=foo type]
Announcement: Value[id:f5c79de7e10ad7c5 Data (type: 0 ): 66 6f 6f ]
 Common bits    InfoHash                       Conn. Get   Ops  IP

>>

I think this is not a desired behavior. Callbacks should called and the associated structures cleared from the search. This PR was written this way in order to exactly not conflict with d44c590 and bd08506.

@aberaud: But, d44c590 and bd08506 exactly do imply [1], doesn't it? I'd appreciate you comment on that so that I can update my fix if need be.

Contributor

sim590 commented Jun 1, 2016

[1] Let table be either the ipv4 or ipv6 table such that table is empty. Announce and Get structures won't be cleared until new nodes are added to the table. Also, done callbacks associated to the table won't even be called with failure status. In fact, after some attempts in dhtnode with ipv6 table empty:

>> ls
Searches:
s:synched, u:updated, a:announced, c:candidate, f:cur req, x:expired, *:known

Search IPv4 99757828bd57b564759d10e1d85cb9dc34179b6d gets: 0, age: 181 s [done] [synced]
 Common bits    InfoHash                       Conn. Get   Ops  IP
  1 e2216ad245ffb28bf7da1ec38f309c3cc1ecd8d9 * [  ] [u ] 192.168.1.100:5000

Search IPv4 9bb095aebe3939cf322054c0f0048863ab073b17 gets: 0, age: 86 s [done] [synced]
 Common bits    InfoHash                       Conn. Get   Ops  IP
  1 e2216ad245ffb28bf7da1ec38f309c3cc1ecd8d9 * [  ] [u ] 192.168.1.100:5000

Search IPv6 9bb095aebe3939cf322054c0f0048863ab073b17 gets: 5, age: -9223156897 s [not synced]
Queries:
Query[SELECT * ]
Query[SELECT * ]
Query[SELECT *  WHERE id=5]
Query[SELECT *  WHERE id=17710297678896355269]
Query[SELECT *  WHERE user_type=foo type]
Announcement: Value[id:f5c79de7e10ad7c5 Data (type: 0 ): 66 6f 6f ]
 Common bits    InfoHash                       Conn. Get   Ops  IP

>>

I think this is not a desired behavior. Callbacks should called and the associated structures cleared from the search. This PR was written this way in order to exactly not conflict with d44c590 and bd08506.

@aberaud: But, d44c590 and bd08506 exactly do imply [1], doesn't it? I'd appreciate you comment on that so that I can update my fix if need be.

@aberaud

This comment has been minimized.

Show comment
Hide comment
@aberaud

aberaud Jun 1, 2016

Contributor

All searches should end and the done callback called asap, if there is no node they should fail immediately, so we should find another way to postpone operations performed before the first node is contacted or have answered.

Contributor

aberaud commented Jun 1, 2016

All searches should end and the done callback called asap, if there is no node they should fail immediately, so we should find another way to postpone operations performed before the first node is contacted or have answered.

@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 1, 2016

Contributor

Taking another look at this and after our discussion, the changes in d44c590 and bd08506 that introduced a bug were to solve a race condition where SecureDht would put its certificate on the network while routing table bootstraping was not complete yet. So, in order to solve all of this in a clean way, the option to make a permanent put is introduced (back). This should solve the race condition problem. The changes in d44c590 and bd08506 that introduced the bug were reverted.

@aberaud: any comment?

Contributor

sim590 commented Jun 1, 2016

Taking another look at this and after our discussion, the changes in d44c590 and bd08506 that introduced a bug were to solve a race condition where SecureDht would put its certificate on the network while routing table bootstraping was not complete yet. So, in order to solve all of this in a clean way, the option to make a permanent put is introduced (back). This should solve the race condition problem. The changes in d44c590 and bd08506 that introduced the bug were reverted.

@aberaud: any comment?

Show outdated Hide outdated src/dht.cpp
@sim590

This comment has been minimized.

Show comment
Hide comment
@sim590

sim590 Jun 3, 2016

Contributor

Merged. See �54a4327.

Contributor

sim590 commented Jun 3, 2016

Merged. See �54a4327.

@sim590 sim590 closed this Jun 3, 2016

@sim590 sim590 deleted the sim590:fix-donecb-not-called branch Jun 3, 2016

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