Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: always send the result channel when triggering a refresh #425

Merged
merged 1 commit into from
Dec 13, 2019
+42 −5
Merged

fix: always send the result channel when triggering a refresh #425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions dht_bootstrap.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,9 +189,6 @@ func (dht *IpfsDHT) Bootstrap(_ context.Context) error {
// error and close. The channel is buffered and safe to ignore.
func (dht *IpfsDHT) RefreshRoutingTable() <-chan error {
res := make(chan error, 1)
select {
case dht.triggerRtRefresh <- res:
default:
}
dht.triggerRtRefresh <- res
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the triggerRtRefresh channel buffered?

Copy link
Contributor

@aarshkshah1992 aarshkshah1992 Dec 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not. But, the reader drains all the reqs on the channel before proceeding so it can signal the result of the refresh to all "simultaneous" callers.

However, what could happen here is that if a refresh is already in progress, all callers of RefreshRoutingTable will block till the current refresh round finishes and then trigger yet another refresh.

@Stebalien Are we okay with that ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the intended behavior, at least for the moment. I want to be able to:

  1. Connect to a peer.
  2. Trigger a refresh that uses that peer.

That means I want the refresh to start after I call the function and I don't want to count any refreshes that started before.

return res
}
42 changes: 41 additions & 1 deletion dht_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,6 @@ func connect(t *testing.T, ctx context.Context, a, b *IpfsDHT) {
}

func bootstrap(t *testing.T, ctx context.Context, dhts []*IpfsDHT) {

ctx, cancel := context.WithCancel(ctx)
defer cancel()

Expand All @@ -212,6 +211,47 @@ func bootstrap(t *testing.T, ctx context.Context, dhts []*IpfsDHT) {
}
}

// Check to make sure we always signal the RefreshRoutingTable channel.
func TestRefreshMultiple(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()

dhts := setupDHTS(t, ctx, 5)
defer func() {
for _, dht := range dhts {
dht.Close()
defer dht.host.Close()
}
}()

for _, dht := range dhts[1:] {
connect(t, ctx, dhts[0], dht)
}

a := dhts[0].RefreshRoutingTable()
time.Sleep(time.Nanosecond)
b := dhts[0].RefreshRoutingTable()
time.Sleep(time.Nanosecond)
c := dhts[0].RefreshRoutingTable()

// make sure that all of these eventually return
select {
case <-a:
case <-ctx.Done():
t.Fatal("first channel didn't signal")
}
select {
case <-b:
case <-ctx.Done():
t.Fatal("second channel didn't signal")
}
select {
case <-c:
case <-ctx.Done():
t.Fatal("third channel didn't signal")
}
}

func TestValueGetSet(t *testing.T) {
ctx, cancel := context.WithCancel(context.Background())
defer cancel()
Expand Down