Dependency context.Context #31
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The following reworks the dependency engine to bring in a real context.Context. The fallout of this means that we decouple the concept of cancelation and where you get the dependency from. In doing so, context has become snapshot. The original comments around this type just state that it is a snapshot, so we might as well just call it that. The dependency Context becomes split. Removing the cross cutting concerns. The aborting of a worker in the start now becomes context.Context and the retrival of a dependency is now a Container. Currently we're just creating a adhoc context.Context, but in the next commit, we'll tie this to the tomb directly, ensuring that upon destruction of the engine, all manifolds will be killed.
The context creation is now tied to the lifecycle of the tomb. Doing this ensures that when the engine is killed, then the context is also killed. It worth noting that cancel is idempotent so, calling it multiple times does nothing. As the main loop on tomb death is also attempting to cancel the context of the snapshot worker, it essentially becomes a no-op.
Closed
jameinel
approved these changes
Sep 14, 2023
| // the tomb. | ||
| func (engine *Engine) scopedContext() (context.Context, context.CancelFunc) { | ||
| ctx, cancel := context.WithCancel(context.Background()) | ||
| return engine.tomb.Context(ctx), cancel |
There was a problem hiding this comment.
reading the engine.tomb.Context() documentation, wouldn't it make more sense to just use engine.tomb.Context() ?
I guess you're trying to get access to its cancel function.
Renaming Container (DI) to Getter to prevent any confusion over what a container is in this context.
This is a breaking change, so we should bump from v3 to v4.
|
/merge |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The following reworks the dependency engine to bring in a real
context.Context. The fallout of this means that we decouple the
concept of cancelation and where you get the dependency from. In
doing so, context has become snapshot. The original comments
around this type just state that it is a snapshot, so we might as
well just call it that.
The dependency Context becomes split. Removing the cross-cutting
concerns. The aborting of a worker in the start now becomes
context.Context and the retrieval of a dependency are now a Container.
The context creation is now tied to the lifecycle of the tomb. Doing
this ensures that when the engine is killed, then the context is also
killed. It is worth noting that cancel is idempotent so, calling it multiple
times does nothing after the first call (unlike channels). As the main
loop on tomb death is also attempting to cancel the context of the
snapshot worker, it essentially becomes a no-op.
JUJU-4623