Ensure that editing fetched Attributes is race-free in any case. #1033
Conversation
|
Fixes #1032 |
Signed-off-by: Andreas Bergmeier <andreas.bergmeier@otto.de>
AndreasBergmeier6176
force-pushed
the
fix_attributes
branch
from
02db1ac
to
06a7052
Compare
|
Thx for the PR! Definitely a possibility for races there. However, your code is technically still not race free because during your copy operation of the map, the map could still be modified. IMHO the best option here would be to add a comment on the functions that the passed map is not supposed to be modified concurrently/not protected during concurrent operations. So callers can pass a copy of the map before using |
FWIU, that would then be a race problem introduced by the call site - AKA not our problem.
Well I think that is common sense: "As long as a call is processing, don't modify the arguments". And actually this PR is necessary because the implementation violated that common sense by storing the map (pointer).
Nope. Then again it is perhaps not by the lack of sdk-go code trying. I mean there is a lot of modifying maps between functions and ownership/concurrency was not very clear to me on first glance. |
| cp[k] = v | ||
| } | ||
| attrs = cp | ||
| } | ||
| return context.WithValue(ctx, withCustomAttributes{}, attrs) |
There was a problem hiding this comment.
question: how about making this a no-op instead?
There was a problem hiding this comment.
Sorry, I am not following. No-op instead of what?
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
question: is it also ok to return nil instead of allocating? Not sure how PubSub handles nil maps though.
There was a problem hiding this comment.
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.
| cp[k] = v | ||
| } | ||
| attrs = cp | ||
| } | ||
| return context.WithValue(ctx, withCustomAttributes{}, attrs) |
There was a problem hiding this comment.
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.
| cp[k] = v | ||
| } | ||
| attrs = cp | ||
| } | ||
| return context.WithValue(ctx, withCustomAttributes{}, attrs) |
There was a problem hiding this comment.
| return context.WithValue(ctx, withCustomAttributes{}, attrs) | |
| return |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
No description provided.