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

fix: introduced RWMutex to flag state to prevent concurrent r/w of map #370

Merged
merged 2 commits into from
Feb 3, 2023

Conversation

skyerus
Copy link
Contributor

@skyerus skyerus commented Feb 3, 2023

This PR

  • Introduces RWMutex on flag state to prevent concurrent read/write of map.

Related Issues

Fixes #368

Notes

Follow-up Tasks

How to test

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
@Kavindu-Dodan
Copy link
Contributor

Kavindu-Dodan commented Feb 3, 2023

I reviewed the change. This is fine for the current implementation. However, we have to use two mutexes that protect the same underlying data structure (Flags backed by the Map). IMO, this shows we need to improve the contract between JsonEvaluator & Flags (contents of json_evaluator_model.go).

I see we need to isolate Flags to a store and perform the required synchronizations there (i.e- Store consumers such as JsonEvaluator must know nothing about the internals of the store). And this means we need a contract similar to below,

image

This will be a big refactoring and will require some changes. But it will help us with maintaining, debugging and adding new features in the future (ex:- changing the store implementation)

What do you think?

cc @james-milligan @AlexsJones @toddbaert

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

I agree with @Kavindu-Dodan that we should open a new issue to improve some of these abstractions, but we should merge this to fix the bug.

@Kavindu-Dodan
Copy link
Contributor

I agree with @Kavindu-Dodan that we should open a new issue to improve some of these abstractions, but we should merge this to fix the bug.

Let's focus on this improvement through #371

skyerus pushed a commit that referenced this pull request Feb 6, 2023
🤖 I have created a release *beep* *boop*
---


##
[0.3.5](v0.3.4...v0.3.5)
(2023-02-06)


### Features

* flagd image signing
([#338](#338))
([eca6a60](eca6a60))
* update in logging to console and Unify case usage, seperators and
punctuation for logging
([#322](#322))
([0bdcfd2](0bdcfd2))


### Bug Fixes

* **deps:** update module github.com/bufbuild/connect-go to v1.5.1
([#365](#365))
([e25f452](e25f452))
* **deps:** update module github.com/open-feature/open-feature-operator
to v0.2.28 ([#342](#342))
([e6df80f](e6df80f))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.14.2
([#336](#336))
([836d3cf](836d3cf))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.14.3
([#372](#372))
([330ac91](330ac91))
* **deps:** update module sigs.k8s.io/controller-runtime to v0.14.4
([#374](#374))
([d90e561](d90e561))
* fix unbuffered channel blocking goroutine
([#358](#358))
([4f1905a](4f1905a))
* introduced RWMutex to flag state to prevent concurrent r/w of map
([#370](#370))
([93e356b](93e356b))
* use event.Has func for file change notification handling (increased
stability across OS)
([#361](#361))
([09f74b9](09f74b9))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Dec 2, 2023
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.

[BUG] fatal error: concurrent map read and map write
4 participants