Thread unsafe lazy initialization in PostActionType

While using discourse in a multi-threaded Ruby implementation and server (TruffleRuby/Puma), errors are produced by the unsafe hash usage in flag settings and this lazy initialization pattern needs to avoid assigning twice:

3 Likes

Does a simple call to PostActionType.flag_settings in an initializer resolve the issue? (eg: just insert here: discourse/000-mini_sql.rb at 3e0cb8ea47ea27cb3b564ac10656884739e4d78c · discourse/discourse · GitHub temporarily)

I guess we could synchronize this block as well. Are there any other big red flags truffle is raising?

2 Likes

Yes, that should work.
Is there any reason to initialize this lazily?
Initializing it eagerly would avoid any synchronization issue and be slightly faster to access as well as simpler.

1 Like

Honestly I am not sure if this lazy init makes sense at all here as well, we have a hook for plugins to replace the entire set of flags, but that would fire prior to the initializer.

@roman any concerns with an eager init here? I guess the trivial fix is:

class << self

  def flag_settings
     ...
   end 
end

flag_settings 

I am thinking that the big reason this happened is that ruby does not ship a clean pattern for class initializers so it pushes devs towards inventing own/using lazy.

2 Likes

It could also be done like this, that would notably avoid reading the ivar twice and make it clearer it’s eager-initialized (the logic could also be in a initial_flag_settings helper method for organization of course):

  class << self
    attr_reader :flag_settings
  end

  @flag_settings = FlagSettings.new
  @flag_settings.add(
    3,
    :off_topic,
    notify_type: true,
    auto_action_type: true,
  )
  # ...
2 Likes

Yeah, I think eager initializing would be safe here. It also shouldn’t conflict with the plugin API, which is rarely used.

3 Likes

Here’s a PR:

4 Likes

This topic was automatically closed after 4 days. New replies are no longer allowed.