-
Notifications
You must be signed in to change notification settings - Fork 4
Add methods to create a handler context and extract links from context #44
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
Conversation
nexus/server.go
Outdated
hctx := ctx.Value(handlerCtxKey).(*handlerCtx) | ||
hctx.mu.Lock() | ||
defer hctx.mu.Unlock() | ||
return hctx.links |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be copied on return (new slice/array + https://pkg.go.dev/builtin#copy is quite cheap especially for small uses) and there should be a set links too
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Are you worried about users mutating? Is it typical for Go libraries to protect users this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes and yes, e.g. https://github.com/grpc/grpc-go/blob/v1.70.0/metadata/metadata.go#L197. This is a very common approach for returning mutable collections that are rarely asked for (e.g. only once at the end in most cases). Also note the mutex isn't protecting the slice only the field (so if someone does AddHandlerLinks
later you could get a race with a reader here).
// HandlerLinks retrieves the attached links on the given handler context. The returned slice should not be mutated. | ||
// The context provided must be the context passed to the handler. | ||
func HandlerLinks(ctx context.Context) []Link { | ||
hctx := ctx.Value(handlerCtxKey).(*handlerCtx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hctx := ctx.Value(handlerCtxKey).(*handlerCtx) | |
hctx, ok := ctx.Value(handlerCtxKey).(*handlerCtx) | |
if !ok { | |
return nil | |
} |
Or if panic is intentional, document that it can panic if called outside of context (and possibly provide a way to know whether you're in a handler context). Same comment applies to AddHandlerLinks
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can document, the panic is intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider a utility to let a user know a context is a handler context then. Basically general utility users should be able to know whether something they are about to call will panic (and therefore can prevent it with a conditional)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that makes sense.
nexus/server.go
Outdated
hctx.mu.Unlock() | ||
cpy := make([]Link, len(links)) | ||
copy(cpy, links) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hctx.mu.Unlock() | |
cpy := make([]Link, len(links)) | |
copy(cpy, links) | |
cpy := make([]Link, len(links)) | |
copy(cpy, links) | |
hctx.mu.Unlock() |
The lock needs to be held while it is accessed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? We always assign to h.links
, we never mutate it directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a common misunderstanding in Go that append
doesn't mutate the first param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I wasn't aware.
Follow up to #42, this is so we can use
AddHandlerLinks
in the Temporal SDK.