-
Notifications
You must be signed in to change notification settings - Fork 100
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
chore: no lifecycle context to shutdown ProviderQueryManager #734
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
@@ Coverage Diff @@
## main #734 +/- ##
=======================================
Coverage 60.42% 60.43%
=======================================
Files 245 245
Lines 31063 31069 +6
=======================================
+ Hits 18771 18775 +4
Misses 10623 10623
- Partials 1669 1671 +2
|
fbfb5d7
to
907d09e
Compare
I understand the part about auto-starting it... but removing the context? It is nice that things die on context cancellation by themselves. |
@hsanjuan IMO Contexts are best for request-scoped operations. For globally shared resources, traditional patterns may be better. Passing a context info Also, passing the context into |
Interesting! I've somehow always considered that contexts could be used for objects' lifecycle management because deriving a context and cancelling the mother of all contexts facilitated cascading cleanup of things, rather than remembering to Close() here and there. Thanks for the pointer! I will not discuss The Go Way :) |
That is still a valid use for context, I just have a particular opinion. I think we need a consistent design paradigm that we can all mostly agree on. So, I pose it as a question to the maintainers here: Do we generally want to limit the scope of a context to that of a request or session, or do we also want to use contexts to for objects' lifecycle management? |
Use Close function instead. Also, do not require calling Startup separately from new.
907d09e
to
e976801
Compare
(We can merge and discuss this later, I think this is an improvement and shouldn't stay behind waiting on discussions) |
Call
Close
instead of cancelling long-lived context.