OnManagedConnectionFailed thread/memory leak fix #1710
Conversation
|
@alexSatov, if the timer iterations are overlapping, wouldn't we still have the problem after this change? |
|
As I wrote, this change does not allow timer iterations to overlap. Arguments (0, 0) guarantee a single iteration, so I see no point in "resource lock". |
|
You're right about not overlapping - my bad, now I see the original interval was also changed to 0. |
|
@eduardobr , you can add a delay it doesn't matter. In this change a working timer will start a new iteration only after previous one has been completed. |
| @@ -2535,7 +2538,8 @@ internal void OnManagedConnectionFailed(object sender, ConnectionFailedEventArgs | |||
| /// <param name="log">Log to write to, if any</param> | |||
| internal void SwitchMaster(EndPoint switchBlame, ConnectionMultiplexer connection, TextWriter log = null) | |||
| { | |||
| if (log == null) log = TextWriter.Null; | |||
| // useless memory allocations? | |||
| // if (log == null) log = TextWriter.Null; | |||
There was a problem hiding this comment.
Can we revert this section please, scoping the change to only one thing here?
| }, null, TimeSpan.FromSeconds(0), TimeSpan.FromSeconds(1)); | ||
| finally | ||
| { | ||
| connection.sentinelMasterReconnectTimer?.Change(TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(0)); |
There was a problem hiding this comment.
If we're disabling period pulling, the second arg should be Timeout.InfiniteTimeSpan here :)
There was a problem hiding this comment.
Yep! -1 is enough. And I've found one more mistake. Alex will send new commit next week.
Hi. I faced a problem with 'ConnectionMultiplexer': when it loses connection with Redis cluster, 'OnManagedConnectionFailed' event handler creates timer, which tries to 'SwitchMaster'. Timer ticks every second, but there is PLINQ query in 'GetConfiguredMasterForService' method, that leads to constantly threads starting (threads start faster than they exit). And if disconnect time is long enough, I get thread/memory leak in service.
PR contains hotfix of a problem with timer (timer starts next iteration only after previous iteration complete). But maybe you should take a closer look at PLINQ queries (they seem superfluous).
Also, I commented
TextWriter.Nullcreation in 'SwitchMaster', because it seems like useless memory allocations (you handle nullable LogProxy).