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

Lock when synchronization the state store #275

Merged
merged 5 commits into from
Mar 6, 2024

Conversation

thorkildcognite
Copy link
Contributor

We encountered:

Unexpected error while synchronizing state store: dictionary changed
size during iteration.

In this case, it is from an extractor with its own statestore that builds upon the internal one, which may make this worse, but when reviewing the code it seems we did not lock when inserting the state to raw. This was probably the cause for the above error. In the same way, we were serializing it in the JSON backed state store without locking the underlying dict, which may in theory also cause this.

If we are concerned with long lock times around the insert, we could make a lock, copy the state store, unlock and then insert/iterate over that copy. It is a bit unclear if that is really needed.

We encountered:

> Unexpected error while synchronizing state store: dictionary changed
  size during iteration.

In this case, it is from an extractor with its own statestore that
builds upon the internal one, which may make this worse, but when
reviewing the code it seems we did not lock when inserting the state
to raw. This was probably the cause for the above error. In the same
way, we were serializing it in the JSON backed state store without
locking the underlying dict, which may in theory also cause this.

If we are concerned with long lock times around the insert, we could
make a lock, copy the state store, unlock and then insert/iterate over
that copy. It is a bit unclear if that is really needed.
@thorkildcognite thorkildcognite requested a review from a team as a code owner December 18, 2023 00:37
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Merging #275 (6f5703f) into master (a4e92c8) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #275      +/-   ##
==========================================
+ Coverage   79.07%   79.12%   +0.04%     
==========================================
  Files          22       22              
  Lines        2065     2069       +4     
==========================================
+ Hits         1633     1637       +4     
  Misses        432      432              
Files Coverage Δ
cognite/extractorutils/statestore.py 92.02% <100.00%> (+0.62%) ⬆️

... and 2 files with indirect coverage changes

@mathialo mathialo force-pushed the add-some-more-locking- branch from 72531cb to 6f5703f Compare March 6, 2024 09:35
@mathialo mathialo merged commit b342b6d into master Mar 6, 2024
6 checks passed
@mathialo mathialo deleted the add-some-more-locking-🔒🔓 branch March 6, 2024 09:56
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.

2 participants