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

always try to read ahead by at least 5 blocks in the PBDagReader #5162

Merged
merged 5 commits into from
Jul 17, 2018
Merged

always try to read ahead by at least 5 blocks in the PBDagReader #5162

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
12719a7
use `copy` instead of looping
Stebalien Jun 27, 2018
0344e3e
better handle context cancellations in the PBDagReader
Stebalien Jun 28, 2018
ff3efb5
always prefetch at least 5 blocks ahead
Stebalien Jun 28, 2018
5c0dc6a
test dag reader context cancellation
Stebalien Jun 28, 2018
7fd3404
explain when a promise can be canceled in pbdagreader
Stebalien Jul 6, 2018
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
21 changes: 19 additions & 2 deletions unixfs/io/pbdagreader.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,10 +95,27 @@ func (dr *PBDagReader) precalcNextBuf(ctx context.Context) error {
}

nxt, err := dr.promises[dr.linkPosition].Get(ctx)
if err != nil {
dr.promises[dr.linkPosition] = nil
switch err {
case nil:
case context.DeadlineExceeded, context.Canceled:
err = ctx.Err()
if err != nil {
return ctx.Err()
}
// In this case, the context used to *preload* the node has been canceled.
// We need to retry the load with our context and we might as
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we elaborate a bit more on this? I had the impression we're always using the same Context from the PBDagReader structure. In which case the local ctx won't be cancelled but the one from the NodePromise will be? The context from the promise comes from this function, maybe in another call (at it may have been preloaded before), but at which point can the global context from the DAG reader change?

Copy link
Member Author

Choose a reason for hiding this comment

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

We're using the context from the call Read call. IMO, that's the correct context as it allows us to cancel reads and seek elsewhere (canceling any associated preloading as well).

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, scratch that. If the user is using Read, we use the reader's cotnext. If the user is manually calling CtxReadFull (a context aware read), we use the supplied context. I don't really think any additional comments will help as the context is just whatever gets passed in (and listing every possible context is just going to lead to comment rot when that changes).

Copy link
Contributor

@schomatis schomatis Jul 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

(and listing every possible context is just going to lead to comment rot when that changes).

It seems to me there are only (or mainly) two possibilities. The most common (by far I think) is that the reader's context is used everywhere, so in fact there is only one context (and this part of the code isn't executed). The second, and less common,

If the user is manually calling CtxReadFull

which is when this scenario takes place (if I understand correctly).

Clarifying that distinction seems worth it, IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough.

// well preload some extra nodes while we're at it.
dr.preload(ctx, dr.linkPosition)
nxt, err = dr.promises[dr.linkPosition].Get(ctx)
dr.promises[dr.linkPosition] = nil
if err != nil {
return err
}
default:
return err
}
dr.promises[dr.linkPosition] = nil
Copy link
Member Author

Choose a reason for hiding this comment

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

Always remove the promise. There's no reason to leave this around.


dr.linkPosition++

switch nxt := nxt.(type) {
Expand Down