Skip to content
This repository has been archived by the owner on Jun 8, 2022. It is now read-only.

minor fix datarace #257

Merged
merged 1 commit into from
Oct 19, 2020
Merged

minor fix datarace #257

merged 1 commit into from
Oct 19, 2020

Conversation

wonderflow
Copy link
Member

No description provided.

Signed-off-by: 天元 <jianbo.sjb@alibaba-inc.com>
Copy link
Member

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

lgtm

@@ -133,38 +133,39 @@ func newTrue() *bool {

func (c *ComponentHandler) createControllerRevision(mt metav1.Object, obj runtime.Object) bool {
curComp := obj.(*v1alpha2.Component)
diff, curRevision := c.IsRevisionDiff(mt, curComp)
comp := curComp.DeepCopy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems the controller runtime already returns a copy from the informer queue(client go, tools/cache/delta_fifo.go:486), is it?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm afraid not, currently vela complains the error: https://github.com/oam-dev/kubevela/runs/1272425015

After debug, I found out it because of this not copy.

Copy link
Member

Choose a reason for hiding this comment

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

Well, the event handler passes the object reference directly:

// OnAdd creates CreateEvent and calls Create on EventHandler
func (e EventHandler) OnAdd(obj interface{}) {
	...
	// Pull the runtime.Object out of the object
	if o, ok := obj.(runtime.Object); ok {
		c.Object = o
	} 

@hongchaodeng hongchaodeng merged commit 723f8ec into crossplane:master Oct 19, 2020
@wonderflow wonderflow deleted the fixrace branch October 19, 2020 05:16
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants