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

Improve goka.Context docs #303

Merged
merged 2 commits into from
Mar 10, 2021
Merged

Improve goka.Context docs #303

merged 2 commits into from
Mar 10, 2021

Conversation

adw1n
Copy link
Contributor

@adw1n adw1n commented Mar 2, 2021

Please squash on merge.

context.go Outdated
@@ -123,14 +123,14 @@ type Context interface {

// Context returns the underlying context used to start the processor or a
// subcontext.
// *Important*: this context is only safe to use within this goroutine, so do not pass it to an
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked the commit that added DeferCommit, and it was not moved and actually intended to be used at DeferCommit. Just the wording is a bit confusing.

Calling DeferCommit usually means you need some asynchronous call, otherwise you wouldn't need DeferCommit.
What this comment intended to mean: Do not pass the goka.Context into that asynchronous call, because it's not valid to be used anymore.
The context.Context that is available by ctx.Context() however, can safely be passed to asynchronous code, and should be, to make it stop when the processor shuts down, it's a child-context of the processor's context.

For example, the processor needs to call a grpc function, but the processor should not be blocked waiting for the call.
(Regardless if that's a good practice or not :)...)

// GOOD:
func handleMsg(ctx goka.Context, msg interface{}){
    // ...
    commit := ctx.DeferCommit()
    callbackCtx := ctx.Context()
    go func(){
        err := grpcClient.Foo(callbackCtx, ...)
        commit(err)
    }()
}

So here we're passing the context.Context of the goka.Context into the goroutine, so the grpc-client can abort the call when the processor shuts down, so the context will appropriately finished (successfully or not). We do not use goka.Context inside the goroutine.

How it should be NEVER done:

// BAD:
func handleMsg(ctx goka.Context, msg interface{}){
    // ...
    commit := ctx.DeferCommit()
    go func(){
        // Emit/Value/SetValue/Join/Lookup etc. lead to inconsistent results and break the general contracts of goka
        ctx.Emit(...)
        commit(...)
    }()
}

That's what the comment was supposed to mean.
So maybe leave the comment at DeferCommit, would the following wording be a bit more clear?

// *Important*: the context where `DeferCommit` is called, is only safe to use within this callback, 
// never pass it into asynchronous code or goroutines
...

Feel free to change or come up with something new.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

// Important: this context is only safe to use within this goroutine, so do not pass it to an
// asynchronously called callback.

This comment left me scratching my head for the following reasons:

  • "this context" - what context are they referring to? Was it a typo and they meant to say callable?
  • If the context is not safe to be passed is the callable also not safe to be passed?
  • Why are they talking about the context here? Wouldn't goka.Context/goka.Context::Context be a better place to document that?

The context.Context that is available by ctx.Context() however, can safely be passed to asynchronous code, and should be, to make it stop when the processor shuts down, it's a child-context of the processor's context.

This is great! Let's document that.

would the following wording be a bit more clear?

Much better although it repeats:

// The context is only valid within the callback, do not store it or pass it to other goroutines.

@adw1n adw1n requested a review from frairon March 4, 2021 16:33
Copy link
Contributor

@frairon frairon left a comment

Choose a reason for hiding this comment

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

sounds good to me

@frairon frairon merged commit b664096 into lovoo:master Mar 10, 2021
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.

None yet

2 participants