Skip to content
Closed
Changes from all commits
Commits
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
78 changes: 66 additions & 12 deletions routing.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,33 +143,85 @@ func (dht *IpfsDHT) GetValue(ctx context.Context, key string) ([]byte, error) {
}

func (dht *IpfsDHT) GetValues(ctx context.Context, key string, nvals int) ([]routing.RecvdVal, error) {
var resChan chan *routing.RecvdVal = make(chan *routing.RecvdVal, 0)
go dht.getValuesAsyncRoutine(ctx, key, nvals, resChan, nil)

var vals []routing.RecvdVal
loop:
for {
select {
case res := <-resChan:
if res == nil {
break loop
} else if res.Error != nil {
return nil, res.Error
} else {
vals = append(vals, *res)
}
}
}

return vals, nil
}

func (dht *IpfsDHT) GetValuesAsync(ctx context.Context, key string, nvals int) <-chan *routing.RecvdVal {
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

whyrusleeping:

Maybe we want to return a channel of 'maybe values' that have either a RecvdVal or an error? Might make the handling of errors a bit simpler

mildred:

Yes, perhaps, but I didn't know so I took the same approach as FindProvidersAsync. I'm ready to change that if you think so.

mildred/ipfs-objects@39c20b2#diff-50d2a8e6d11e785b103932ac6c926e72R169

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You added the error field into the RecvdVal in the routing interface PR. Should we make use of it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh, I see getValuesAsyncRoutine will use it if error channel is not specified. Should we use it instead?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It will use both

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mildred is the errChan ever used? It seems a bit redundant at this point

var resChan chan *routing.RecvdVal = make(chan *routing.RecvdVal, 0)
var errChan chan error = make(chan error, 0)
go dht.getValuesAsyncRoutine(ctx, key, nvals, resChan, errChan)
go func() {
for err := range errChan {
log.Debugf("Query error: %s", err)
notif.PublishQueryEvent(ctx, &notif.QueryEvent{
Type: notif.QueryError,
Extra: err.Error(),
})
}
}()
return resChan
}

func (dht *IpfsDHT) getValuesAsyncRoutine(ctx context.Context, key string, nvals int, resChan chan<- *routing.RecvdVal, errChan chan<- error) {
var valslock sync.Mutex
var sentRes int

defer close(resChan)
if errChan != nil {
defer close(errChan)
}

// If we have it local, dont bother doing an RPC!
lrec, err := dht.getLocal(key)
if err == nil {
// TODO: this is tricky, we dont always want to trust our own value
// what if the authoritative source updated it?
log.Debug("have it locally")
vals = append(vals, routing.RecvdVal{
sentRes = sentRes + 1
resChan <- &routing.RecvdVal{
Val: lrec.GetValue(),
From: dht.self,
})
}

if nvals <= 1 {
return vals, nil
if nvals == 0 || nvals == 1 {
return
}
} else if nvals == 0 {
return nil, err
if errChan != nil {
errChan <- err
}
resChan <- &routing.RecvdVal{Error: err}
return
}

// get closest peers in the routing table
rtp := dht.routingTable.NearestPeers(kb.ConvertKey(key), KValue)
log.Debugf("peers in rt: %d %s", len(rtp), rtp)
if len(rtp) == 0 {
log.Warning("No peers from routing table!")
return nil, kb.ErrLookupFailure
if errChan != nil {
errChan <- err
}
resChan <- &routing.RecvdVal{Error: kb.ErrLookupFailure}
return
}

// setup the Query
Expand Down Expand Up @@ -205,11 +257,12 @@ func (dht *IpfsDHT) GetValues(ctx context.Context, key string, nvals int) ([]rou
Val: rec.GetValue(),
From: p,
}
resChan <- &rv
valslock.Lock()
vals = append(vals, rv)
sentRes += 1

// If weve collected enough records, we're done
if len(vals) >= nvals {
if sentRes >= nvals || nvals >= 0 {
res.success = true
}
valslock.Unlock()
Expand All @@ -226,14 +279,15 @@ func (dht *IpfsDHT) GetValues(ctx context.Context, key string, nvals int) ([]rou

// run it!
_, err = query.Run(ctx, rtp)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

whyrusleeping:

I think its probably bad that we ignore this error if we got at least one response back... I think this should be addressed separately

mildred:

Why do you say it's ignored? The fact is we must respond asynchronously to errors, and this one will appear at the very last moment. We cannot return it directly, this function is in a goroutine.

mildred/ipfs-objects@39c20b2#diff-50d2a8e6d11e785b103932ac6c926e72R274

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In the case where sentRes != 0, we never return the error from Run

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

good catch

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

A few days ago, I understood more from your remark. I just fixed an issue where sentRes wasn't incremented when needed (it collects the number of results sent).

However, when there is no result, we might get an error from the lower layers, but that doesn't necessarily mean the GetValues must return an error. Instead it might mean that there is no result at all and we must not trigger an error. The previous version of the code silents some errors in this case, I see no reason to change that in this PR.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@mildred right, i was mostly commenting that the existing behaviour might be incorrect. We can always change it in a different PR though.

if len(vals) == 0 {
if sentRes == 0 {
if err != nil {
return nil, err
if errChan != nil {
errChan <- err
}
resChan <- &routing.RecvdVal{Error: err}
}
}

return vals, nil

}

// Value provider layer of indirection.
Expand Down