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.

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

Clarify response completion #304

Merged
merged 2 commits into from
Dec 9, 2021
Merged

Conversation

hannahhoward
Copy link
Collaborator

Goals

There is a lot of confusion in the ResponseManager about when we're actually "done" with the request. This is cause the query executor completes before we've truly finished sending data -- that only happens once the message queue confirms everything has gone over the wire.

As a result it's very unclear in the code where a request ends -- has it gone over the wire or is it just done processing queries. Even most ways a request can abort still have a response code sent, meaning we need to wait till that gets sent to cleanup the request.

What's worse, cleanup tasks are spread out every where, with somewhat inconsistent implementation.

Implementation

The main change here is to add a "CompletingSend" status that tracks the time from when the query finishes executing to when the last message goes over the wire.

This makes it clearer if we're DONE DONE or still waiting for confirmation from the network queue.

We also add a method terminateRequest that cleans up the response when it's truly done, being careful to
only delete requests when they finish going over the network.

Put requests that are not processing but still going over the network in a state of CompletingSend.

only delete requests when they finish going over the network. put requests that are not processing
but still going over the network in a state of CompletingSend
@hannahhoward hannahhoward force-pushed the feat/simplify-response-termination branch from 4315027 to 3740e98 Compare December 9, 2021 02:18
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

👌 very nice!
The main thing I don't follow here though is how we get from the dangling graphsync.CompletingSend = true states (bottom of finishTask() being the main one) to a final terminateRequest(). Is it guaranteed that we'll end up in terminateRequest() from there via responsemanager/subscriber.go#OnNext() having sent the data? Are there other paths that might get us there and is there any way that it could remain in that state and not be properly terminated? (Which I guess the new diagnostics might tell us in practice!).

@hannahhoward
Copy link
Collaborator Author

@rvagg

So: OnNext would only not fire in a case where everything was shutting down (i.e. the global graphsync context got cancelled. And it wouldn't matter anyway then :). Anyway, if there are other times, this is how we'll find that out!

@hannahhoward hannahhoward merged commit 3bd6478 into main Dec 9, 2021
hannahhoward added a commit that referenced this pull request Dec 9, 2021
feat(responsemanager): clarify response completion (#304)

only delete requests when they finish going over the network. put requests that are not processing
but still going over the network in a state of CompletingSend

feat(taskqueue): fix race on peer state gather (#303)
@mvdan mvdan deleted the feat/simplify-response-termination branch December 15, 2021 14:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants