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

Revisit Shutdown Behavior #220

Open
hannahhoward opened this issue Sep 24, 2021 · 0 comments
Open

Revisit Shutdown Behavior #220

hannahhoward opened this issue Sep 24, 2021 · 0 comments
Labels
effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important need/maintainer-input Needs input from the current maintainer(s) need/triage Needs initial labeling and prioritization needs definition means this story needs further data before it can be estimated P2 Medium: Good to have, but can wait until someone steps up

Comments

@hannahhoward
Copy link
Collaborator

hannahhoward commented Sep 24, 2021

We have pretty terrible patterns for shutting down all over the code base.

Currently, you pass a context to Graphsync when you start it and the only way to shutdown is just cancel the context and hope all the go routines get cleaned up (I think they do but...)

Meanwhile, we actually have Shutdown methods for lots of the components but all they do is shut down an internal context... and they're not even exposed publicly.

Per @mvdan :

If we went with the "we'll eventually want to block until all request goroutines are stopped" route,
then I think these methods should  gain a context parameter, too. That way, I can say "graceful 
shutdown for 10s to stop accepting requests, and after that, kill all requests which aren't done
yet". This is how net/http does it: https://pkg.go.dev/net/http#Server.Shutdown

We should... have a way to shutdown that confirms all the go routines are actually closed.

@hannahhoward hannahhoward added the need/triage Needs initial labeling and prioritization label Sep 24, 2021
@hannahhoward hannahhoward added need/maintainer-input Needs input from the current maintainer(s) needs definition means this story needs further data before it can be estimated P2 Medium: Good to have, but can wait until someone steps up effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important labels Sep 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important need/maintainer-input Needs input from the current maintainer(s) need/triage Needs initial labeling and prioritization needs definition means this story needs further data before it can be estimated P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

No branches or pull requests

1 participant