fix: Avoid use_state race conditions. Remove key argument to use_state
#868
Conversation
Remove key argument from use_state and hardcode it instead. Add unit tests for race conditions. Add lock when accessing the cache or kvs to prevent race conditions.
Pijukatel
added
bug
It is pretty uggly without Barrier.
Pijukatel
changed the title
Pijukatel
changed the title
github-actions
bot
added
the
tested
|
the PR title should be informative about what the change does, "align with JS version" is not gonna tell much to our users, they don't know the JS version |
|
May not be related, but I observed flaky behavior in state handling here |
I will check it. It is supposed to be autosaved when specific event happens. It is supposed to happen every 50ms, so 1s seemed like a long enough reserve. Maybe there is some hard block in some cases and no amount of reserve time is enough? |
Pijukatel
changed the title
use_state race conditions. Remove key argument to use_state.
janbuchar
left a comment
janbuchar
left a comment
There was a problem hiding this comment.
Seems legit, but I want to re-read the tests after my comments are resolved.
| assert await autosaved_within_deadline(key=key_name, expected_value={'hello': 'new_world'}) | ||
|
|
||
|
|
||
| async def test_get_auto_saved_value_auto_save_race_conditions(key_value_store: KeyValueStore) -> None: |
There was a problem hiding this comment.
Does this reliably reproduce the error if you remove the lock?
There was a problem hiding this comment.
I tried stress testing it 1000x times and it failed with old implementation all the time. But I acknowledge that it is not guaranteed and in theory it can pass. Same as in reality if such delay in kvs access occurs, it will most likely end in race condition, but in theoretical case it does not have to.
I did try coming up with test without such weakness, but I failed to find better approach.
This test should not have any false positives and should not be flaky, but has theoretical chance for false negative. In CI we run the same test for various Python implementations which makes already purely theoretical chance completely negligible.
vdusek
changed the title
use_state race conditions. Remove key argument to use_state.use_state race conditions. Remove key argument to use_state
Co-authored-by: Vlada Dusek <[email protected]>
Description
Align Python implementation of
use_statewirth JSAdd lock when accessing the cache or kvs to prevent race conditions.
Add unit tests for race conditions.
Breaking
Remove key argument from
use_stateand hard code it toBasicCrawler.CRAWLEE_STATE_KEY.(Change in experimental feature is not marked as hard breaking with
!.)Issues