Skip to content

feat!: made EvaluationContext fields unexported with a constructor and setters to enforce immutability#91

Merged
skyerus merged 4 commits into
open-feature:mainfrom
skyerus:issue-90_thread-safety
Oct 7, 2022
Merged

feat!: made EvaluationContext fields unexported with a constructor and setters to enforce immutability#91
skyerus merged 4 commits into
open-feature:mainfrom
skyerus:issue-90_thread-safety

Conversation

@skyerus

@skyerus skyerus commented Oct 4, 2022

Copy link
Copy Markdown
Contributor

No description provided.

@skyerus skyerus linked an issue Oct 4, 2022 that may be closed by this pull request
@codecov-commenter

codecov-commenter commented Oct 4, 2022

Copy link
Copy Markdown

Codecov Report

Merging #91 (590dd7d) into main (9fbed88) will increase coverage by 0.43%.
The diff coverage is 83.78%.

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   69.24%   69.68%   +0.43%     
==========================================
  Files           7        8       +1     
  Lines         569      597      +28     
==========================================
+ Hits          394      416      +22     
- Misses        158      164       +6     
  Partials       17       17              
Impacted Files Coverage Δ
pkg/openfeature/evaluation_context.go 80.95% <80.95%> (ø)
pkg/openfeature/client.go 69.41% <87.50%> (+0.03%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

…d setters to enforce immutability

Signed-off-by: Skye Gill <gill.skye95@gmail.com>
@skyerus skyerus force-pushed the issue-90_thread-safety branch from 1f3b60a to a1fbbbc Compare October 6, 2022 12:05
@skyerus skyerus changed the title feat!: made EvaluationContext fields unexported with a constructor and setters to enforce thread safety feat!: made EvaluationContext fields unexported with a constructor and setters to enforce immutability Oct 6, 2022
@skyerus skyerus marked this pull request as ready for review October 6, 2022 12:07
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
Signed-off-by: Skye Gill <gill.skye95@gmail.com>

@james-milligan james-milligan left a comment

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.

Nice work

@skyerus skyerus force-pushed the issue-90_thread-safety branch from 4eb9578 to 68166f3 Compare October 6, 2022 12:33

@toddbaert toddbaert left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks good.

I think the only thing I'm missing here is the lock on global providers and hooks, as well as client hooks. We probably want to read-lock all of these during evaluation, and write lock them during mutation.

@skyerus

skyerus commented Oct 6, 2022

Copy link
Copy Markdown
Contributor Author

This looks good.

I think the only thing I'm missing here is the lock on global providers and hooks, as well as client hooks. We probably want to read-lock all of these during evaluation, and write lock them during mutation.

Great points. I've tackled this in another PR: #93

@toddbaert toddbaert self-requested a review October 6, 2022 16:26
@skyerus skyerus force-pushed the issue-90_thread-safety branch from 68166f3 to 68adbb1 Compare October 7, 2022 11:45
Signed-off-by: Skye Gill <gill.skye95@gmail.com>
@skyerus skyerus force-pushed the issue-90_thread-safety branch from 68adbb1 to 610af7d Compare October 7, 2022 11:53
@skyerus skyerus merged commit 691a1e3 into open-feature:main Oct 7, 2022
@skyerus skyerus deleted the issue-90_thread-safety branch October 7, 2022 14:18
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.

[FEATURE] Ensure thread safety of sdk

4 participants