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

Ensure that editing fetched Attributes is race-free in any case. #1033

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 23 additions & 1 deletion protocol/pubsub/v2/attributes.go
Expand Up @@ -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)
Copy link
Member

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 handles nil maps though.

Copy link
Contributor Author

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.

}

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)
Copy link
Member

Choose a reason for hiding this comment

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

question: how about making this a no-op instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I am not following. No-op instead of what?

Copy link
Member

Choose a reason for hiding this comment

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

Well, right now this code means the context key withCustomAttributes{} is assigned nil if attrs is nil. So I was wondering why not just return without modifying the context instead of setting it to nil (you set the value from attrs which in nil case is in fact nil).

Would still work with your code above, just save a context operation.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return context.WithValue(ctx, withCustomAttributes{}, attrs)
return

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, right now this code means the context key withCustomAttributes{} is assigned nil if attrs is nil. So I was wondering why not just return without modifying the context instead of setting it to nil (you set the value from attrs which in nil case is in fact nil).

This then removes the ability to reset the Attributes saved on the Context. Might not be necessary, IDK.
That being said IF we should do that we def. need to document it because it no longer is a naive/straight-forward implementation.

Copy link
Member

Choose a reason for hiding this comment

The 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 nil is not really valid as attributes, which you work around in your other function.

We could document that passing nil as attributes is a no-op and does not modify the context?
Again, only unless nil is an explicitly allowed/useful value for PubSub?

}