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

Fix error codes #182

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Fix error codes #182

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
3 changes: 2 additions & 1 deletion message/message.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@ func IsTerminalFailureCode(status graphsync.ResponseStatusCode) bool {
status == graphsync.RequestFailedContentNotFound ||
status == graphsync.RequestFailedLegal ||
status == graphsync.RequestFailedUnknown ||
status == graphsync.RequestCancelled
status == graphsync.RequestCancelled ||
status == graphsync.RequestRejected
}

// IsTerminalResponseCode returns true if the response code signals
Expand Down
6 changes: 4 additions & 2 deletions responsemanager/queryexecutor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,14 @@ func (qe *queryExecutor) executeQuery(
code = graphsync.RequestFailedUnknown
return nil
}
if err == errCancelledByCommand {
if err == runtraversal.ErrFirstBlockLoad {
code = graphsync.RequestFailedContentNotFound
} else if err == errCancelledByCommand {
Comment on lines +132 to +134
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like in some of the error cases handled here we set the code then return nil.
But in these new error cases we don't return after setting the code. Just want to confirm that's correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea so, basically the above two cases involve no final network message to send (see rb.ClearRequest)

While these cases do involve a final message back to the requestor. (see rb.FinishWithError). Ultimately they return nil two lines below that. This is a lot of nested conditionals and I agree it's confusing and merits a refactor, but yes, it's correct :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool just wanted to make sure 👍

code = graphsync.RequestCancelled
} else {
code = graphsync.RequestFailedUnknown
}
rb.FinishWithError(graphsync.RequestCancelled)
rb.FinishWithError(code)
} else {
code = rb.FinishRequest()
}
Expand Down
7 changes: 6 additions & 1 deletion responsemanager/querypreparer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,18 @@ func (qe *queryPreparer) prepareQuery(ctx context.Context,
var transactionError error
var isPaused bool
failNotifee := notifications.Notifee{Data: graphsync.RequestFailedUnknown, Subscriber: sub}
rejectNotifee := notifications.Notifee{Data: graphsync.RequestRejected, Subscriber: sub}
err := qe.responseAssembler.Transaction(p, request.ID(), func(rb responseassembler.ResponseBuilder) error {
for _, extension := range result.Extensions {
rb.SendExtensionData(extension)
}
if result.Err != nil || !result.IsValidated {
if result.Err != nil {
rb.FinishWithError(graphsync.RequestFailedUnknown)
rb.AddNotifee(failNotifee)
transactionError = result.Err
} else if !result.IsValidated {
rb.FinishWithError(graphsync.RequestRejected)
rb.AddNotifee(rejectNotifee)
transactionError = errors.New("request not valid")
} else if result.IsPaused {
rb.PauseRequest()
Expand Down
Loading