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-cache-returns-nil #3213

Merged
merged 1 commit into from May 25, 2017
Merged

fix-cache-returns-nil #3213

merged 1 commit into from May 25, 2017

Conversation

@masarakki
Copy link
Contributor

masarakki commented May 22, 2017

In my instance, sometimes user.settings.interactions returns nil .
I understand that this code never returns nil, but something happen...

@masarakki masarakki force-pushed the masarakki:fix-cache-returns-nil branch May 22, 2017
@nightpool

This comment has been minimized.

Copy link
Collaborator

nightpool commented May 22, 2017

Looks like this changes a bunch of existing behavior—see the test failures. Can you outline exactly what the issue is and why it's a problem?

If you say "that this code never returns nil", then you should probably investigate why it's going on not just masking the symptoms.

@masarakki masarakki force-pushed the masarakki:fix-cache-returns-nil branch May 22, 2017
@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented May 22, 2017

Does user.settings.interactions for the same user sometimes return nil, or is it more of an issue of only some users missing defaults?

@masarakki masarakki changed the title fix-cache-returns-nil [WIP] fix-cache-returns-nil May 22, 2017
@masarakki

This comment has been minimized.

Copy link
Contributor Author

masarakki commented May 22, 2017

this happens both case user has some settings / no settings ,

I found what happened on rails console,

irb(main):048:0> Rails.cache.exist?(Setting.cache_key('interactions', user))
=> true
irb(main):049:0> Rails.cache.fetch(Setting.cache_key('interactions', user))
=> nil
# ..few minutes after
irb(main):050:0> Rails.cache.fetch(Setting.cache_key('interactions', user))
=> {"must_be_follower"=>false, "must_be_following"=>false}
@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented May 22, 2017

So the problem is the old result is cached and the cache is not discarded when setting is updated? Or am I still misunderstanding what happens and why?

@masarakki

This comment has been minimized.

Copy link
Contributor Author

masarakki commented May 22, 2017

Essentially, this problem is db_val of 'interactions' or defuallt_values['interactions'] never returns nil, but (it seems that) redis stored nil

app/models/setting.rb Outdated
@@ -35,7 +36,7 @@ def [](key)
default_settings[key]
end
end

val = default_settings[key] unless Rails.cache.exist?(key)

This comment has been minimized.

Copy link
@Gargron

Gargron May 22, 2017

Member

Should this not be if val.nil?

@masarakki

This comment has been minimized.

Copy link
Contributor Author

masarakki commented May 23, 2017

rails c                                [19:13:37]
Loading development environment (Rails 5.0.2)
[1] pry(main)> Settings::ScopedSettings.instance_variable_get(:@object)
=> nil
[2] pry(main)> User.first.settings
  User Load (0.6ms)  SELECT  "users".* FROM "users" ORDER BY "users"."id" ASC LIMIT $1  [["LIMIT", 1]]
=> Settings::ScopedSettings(id: integer, var: string, value: text, thing_type: string, thing_id: integer, created_at: datetime, updated_at: datetime)
[3] pry(main)> Settings::ScopedSettings.instance_variable_get(:@object)
=> #<User id: 1, email:`...>

I think it is the factor of its problem.
I fix to using instance of ScopeSettings, and I'm testing in my instance.

@masarakki masarakki force-pushed the masarakki:fix-cache-returns-nil branch 4 times, most recently May 24, 2017
@masarakki masarakki force-pushed the masarakki:fix-cache-returns-nil branch to 27c3d74 May 24, 2017
@masarakki masarakki changed the title [WIP] fix-cache-returns-nil fix-cache-returns-nil May 24, 2017
@alpaca-tc

This comment has been minimized.

Copy link
Contributor

alpaca-tc commented May 24, 2017

I noticed that current Settings::ScopedSettings is not thread-safe. I might be wrong, but is this the cause? 🤔

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented May 24, 2017

@alpaca-tc Perhaps we should add monitor/synchronize to it? I don't know how to reproduce this issue for testing, so I am just guessing...

@alpaca-tc

This comment has been minimized.

Copy link
Contributor

alpaca-tc commented May 24, 2017

@Gargron I am just guessing, too. I will try to reproduce it. 😕

@alpaca-tc

This comment has been minimized.

Copy link
Contributor

alpaca-tc commented May 24, 2017

@Gargron just finished, we should fix this problem as soon as possible.
I'll try to create pull-request tomorrow 😄

ActiveRecord::Base.logger = Logger.new('/dev/null')
raise 'please enable redis store' unless Rails.cache.is_a?(ActiveSupport::Cache::RedisStore)

TIMES = 50
KEY = 'test'

begin
  threads = []

  # This thread doesn't update settings.
  # If other thread updates record or cache, this thread raise error.
  threads << Thread.new do
    first_user = User.find(1)
    previous = first_user.settings[KEY]

    TIMES.times { first_user.settings; sleep(0.1) } # Overwrite Settings::ScopedSettings's @object to check thread-safe

    current = first_user.settings[KEY]
    raise "Ooops!! settings is updated by other thread. prevous:#{previous}, current:#{current}" unless previous == current
  end

  # Unfortunately, this thread updates settings of first user
  threads << Thread.new do
    second_user = User.find(2)

    TIMES.times do
      new_value = second_user.settings[KEY].to_i + 1
      second_user.settings[KEY] = new_value # UPDATE "settings" SET "value" = $1, "updated_at" = $2 WHERE "settings"."id" = $3
    end
  end

  threads.each(&:join)
rescue => error
  puts error
end
@alpaca-tc

This comment has been minimized.

Copy link
Contributor

alpaca-tc commented May 24, 2017

Ah, it seems that this PR fixed the problem, does not? @masarakki
You need still fix Setting and ScopedSettings.

I hope to merge after our reviewing. how about it? @Gargron

@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented May 24, 2017

Yes I didn't notice the refactor was already in the PR. Still, should I wait for you to confirm this fixes the issue, @alpaca-tc ?

@masarakki

This comment has been minimized.

Copy link
Contributor Author

masarakki commented May 25, 2017

After this patch, Setting is (maybe) used only for global settings.
So I think it's not so dangerous.

I suggest to avoid using rails-settings.

@Gargron Gargron merged commit 3b59f9c into tootsuite:master May 25, 2017
2 checks passed
2 checks passed
codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@masarakki masarakki deleted the masarakki:fix-cache-returns-nil branch May 25, 2017
@Gargron

This comment has been minimized.

Copy link
Member

Gargron commented Jul 19, 2017

I am adding a Redis lock gem in #4258. One of the examples in the gem documentation is using the lock for fetching cache: https://github.com/marioizquierdo/mario-redis-lock/blob/master/EXAMPLE_DOG_PILE_EFFECT.md Perhaps this would be useful for this code? I think it's still a bit buggy, I remember sometimes seeing in my sidekiq logs that some local user's settings which should never be nil return nil and the job fails

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.