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

BUG: Waiting on wrong key #24

Merged
merged 3 commits into from
Jun 25, 2021
Merged

Conversation

tstone
Copy link
Collaborator

@tstone tstone commented Jun 24, 2021

Background

A bug was identified earlier today where the wait_for_new_value method is incorrectly waiting on the wrong key. This bugfix updates the code to re-fetch the latest key value whenever it attempts to see if another thread has written the value. This makes sure that re-attempts are always attempting to get the latest value.

Tasks

  • Code of Conduct reviewed
  • Specs written and passing
  • Backwards-incompatible changes called out in this PR
  • Increment the version file (./lib/atomic_cache/version.rb) when applicable

@tstone tstone added the bug Something isn't working label Jun 24, 2021
return value if value.present?

# wait for the other process if a last known value isn't there
if key.present?
return time('wait.run', tags: tags) do
wait_for_new_value(key, options, tags)
wait_for_new_value(keyspace, options, tags)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

quick_retry and wait_for_new_value changed their approach here a bit:

  • quick_retry now uses the initial key obtained on line 40, as very little time has passed, and it's more efficient for all the locations that have quick_retry disabled.
  • wait_for_new_value no longer takes the key, as time will have elapsed between attempts, such that it should make sure it's looking at the most recent key for a value.

@@ -162,6 +159,8 @@ def wait_for_new_value(key, options, tags)
backoff_duration_ms = option(:backoff_duration_ms, options, backoff_duration_ms)
sleep((backoff_duration_ms.to_f / 1000) * attempt)

# re-fetch the key each time, to make sure we're actually getting the latest key with the correct LMT
key = @timestamp_manager.current_key(keyspace)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line here is effectively the bug fix. Each attempt it will make sure to get the most current key.

How this is actually working in a cold start scenario is that because LMT is empty, the initial key is just prefix.foobar. The value that actually gets written by the generating process however is prefix.foobar.timestamp. The old code would continue to try and read prefix.foobar then exhaust it's attempts. This fix causes it to always grab the most current key, which will cause it to see prefix.foobar.timestamp as written by another generating process, and use that key for a value.

end
end


Copy link
Collaborator Author

@tstone tstone Jun 24, 2021

Choose a reason for hiding this comment

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

be warned: there be dragons below 🐉

🙈

@tstone tstone marked this pull request as ready for review June 24, 2021 21:46
Copy link
Contributor

@lebeerman lebeerman left a comment

Choose a reason for hiding this comment

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

The crux of this update generally makes sense - the integration test is really cool, but noting I'm not 100% about threadsafe-ness and how all the process queue management is working.

@tstone tstone merged commit 6b10412 into main Jun 25, 2021
@tstone tstone deleted the bugfix/MTF-559-waiting-different-keys branch June 25, 2021 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants