[5.0] only register prometheus handlers when prometheus_plugin enabled; fixing memory leak
#2038
Conversation
| @@ -4597,7 +4597,6 @@ namespace eosio { | |||
| // called from any thread | |||
| void connections_manager::start_conn_timers() { | |||
| start_conn_timer(connector_period, {}, timer_type::check); // this locks mutex | |||
| start_conn_timer(connector_period, {}, timer_type::stats); // this locks mutex | |||
There was a problem hiding this comment.
I wonder if this was mistakenly left in during some sort of refactor? connection_statistics_monitor() is clearly only valid when update_p2p_connection_metrics is a valid function (it even assert()s on it). I thought maybe connection_statistics_monitor() drives some other non-prometheus statistics when prometheus isn't enabled, but it doesn't seem so.
| @@ -4597,7 +4597,6 @@ namespace eosio { | |||
| // called from any thread | |||
| void connections_manager::start_conn_timers() { | |||
| start_conn_timer(connector_period, {}, timer_type::check); // this locks mutex | |||
| start_conn_timer(connector_period, {}, timer_type::stats); // this locks mutex | |||
There was a problem hiding this comment.
If this line is removed, won't it prevent the prometheus net_plugin stats from being updated, even if the prometheus plugin is enabled?
Unless I missed some other piece of code calling connections_manager::connection_statistics_monitor, but I didn't find any.
Maybe we should just have that function return immediately if update_p2p_connection_metrics is empty?
There was a problem hiding this comment.
Oh, I missed that, thanks Kevin.
I think it would be safer to check for !!update_p2p_connection_metrics inside connection_statistics_monitor rather than at the calling site, but I guess it works as is.
appbase plugins are constructed no matter if they are enabled or not. This meant no matter whether the
prometheus_pluginwas enabled it would install its handlers in tohttp_plugin,net_plugin, etc. These handlers alwayspost()toprometheus_plugin's strand. But whenprometheus_pluginisn't enabled its strand is never serviced leading to an endlessly growing queue that is never drained. Each HTTP request would add on to this queue, for example.Move the handler registration to
plugin_initialize()so handlers will only be registered when the plugin is enabled ergo when the strand will be pumped. (this is actually how it was prior to #1074, which makes me wonder if there was a need for this change, but I am not immediately seeing one)Resolves #2030