Skip to content

Add locking around ops that change template router state.#8893

Merged
openshift-bot merged 1 commit intoopenshift:masterfrom
ramr:router-p0
May 18, 2016
Merged

Add locking around ops that change template router state.#8893
openshift-bot merged 1 commit intoopenshift:masterfrom
ramr:router-p0

Conversation

@ramr
Copy link
Copy Markdown
Contributor

@ramr ramr commented May 16, 2016

fixes #8825

@liggitt PTAL

return false
}

r.lock.Lock()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Find service unit does not make a copy, so this is not safe.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unless we also guarantee that we make no mutations to state. Is that the case?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, might be better to separate FindServiceUnit into an internal/external part - that way all the internal calls can work thru' in a lock. Let me redo this. Thx

  lockless eventually and use a "shadow" (cloned) copy of the
  config when we do a reload.
o Fixes as per @smarterclayton review comments.
@ramr
Copy link
Copy Markdown
Contributor Author

ramr commented May 16, 2016

[test]

@smarterclayton
Copy link
Copy Markdown
Contributor

Changes look good to me - will wait for clean run and then apply label.

@smarterclayton
Copy link
Copy Markdown
Contributor

smarterclayton commented May 16, 2016 via email

@deads2k
Copy link
Copy Markdown
Contributor

deads2k commented May 17, 2016

re[test]

Another failure in the test flow, still haven't seen one in merge. I think I'll try bumping the timeout. Maybe someone's parallel test-cmd and test-go makes things extra slow.

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin test up to 6a47777

@openshift-bot
Copy link
Copy Markdown
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/3882/)

@smarterclayton
Copy link
Copy Markdown
Contributor

[merge] Lgtm

@openshift-bot
Copy link
Copy Markdown
Contributor

openshift-bot commented May 18, 2016

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5934/) (Image: devenv-rhel7_4225)

@openshift-bot
Copy link
Copy Markdown
Contributor

Evaluated for origin merge up to 6a47777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

router can hit concurrent map read and map write during commit

4 participants