Thread unsafe lazy initialization in PostActionType

Bug
1

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
2

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
3

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
4

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
5

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
6

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

3 Likes
7

Hereā€™s a PR:

4 Likes
8

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