-
Notifications
You must be signed in to change notification settings - Fork 227
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
Ensure that editing fetched Attributes is race-free in any case. #1033
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -14,12 +14,34 @@ import ( | |||||
type withCustomAttributes struct{} | ||||||
|
||||||
func AttributesFrom(ctx context.Context) map[string]string { | ||||||
return binding.GetOrDefaultFromCtx(ctx, withCustomAttributes{}, make(map[string]string)).(map[string]string) | ||||||
ctxVal := binding.GetOrDefaultFromCtx(ctx, withCustomAttributes{}, nil) | ||||||
if ctxVal == nil { | ||||||
return make(map[string]string, 0) | ||||||
} | ||||||
|
||||||
m := ctxVal.(map[string]string) | ||||||
|
||||||
// Since it is possible that we get the same map from one ctx multiple times | ||||||
// we need to make sure, that it is race-free to modify returned map. | ||||||
cp := make(map[string]string, len(m)) | ||||||
for k, v := range m { | ||||||
cp[k] = v | ||||||
} | ||||||
return cp | ||||||
} | ||||||
|
||||||
// WithCustomAttributes sets Message Attributes without any CloudEvent logic. | ||||||
// Note that this function is not intended for CloudEvent Extensions or any `ce-`-prefixed Attributes. | ||||||
// For these please see `Event` and `Event.SetExtension`. | ||||||
func WithCustomAttributes(ctx context.Context, attrs map[string]string) context.Context { | ||||||
if attrs != nil { | ||||||
// Since it is likely that the map gets used in another goroutine | ||||||
// ensure that modifying passed in map is race-free. | ||||||
cp := make(map[string]string, len(attrs)) | ||||||
for k, v := range attrs { | ||||||
cp[k] = v | ||||||
} | ||||||
attrs = cp | ||||||
} | ||||||
return context.WithValue(ctx, withCustomAttributes{}, attrs) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: how about making this a no-op instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I am not following. No-op instead of what? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, right now this code means the context key Would still work with your code above, just save a context operation. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This then removes the ability to reset the Attributes saved on the Context. Might not be necessary, IDK. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair point. However, resetting might not be needed since this is all using context logic which could easily be done by passing a new/fresh (root) context without this key. So I wonder whether we really need the possibility to reset the key? Especially since We could document that passing |
||||||
} |
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.
question: is it also ok to return
nil
instead of allocating? Not sure how PubSub handlesnil
maps though.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.
Probably not because the result immediately gets later assigned to
Attributes
and then the rest of the sdk code just assumes there is a map there.