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:
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?
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.
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.
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, ) # ...
Yeah, I think eager initializing would be safe here. It also shouldn’t conflict with the plugin API, which is rarely used.
Here’s a PR: