On Wed, Nov 6, 2013 at 9:33 PM, Pavel Levshin <[email protected]> wrote:
> > wtpShutdownAll() is called separately for each thread pool, it does much > more than simple signaling. > > wtpAdviseMaxWorkers() is called from several places, but most of them are > related to DA shutdown. One place where it is used for all workers is > qqueueAdviseMaxWorkers(). And there, it is called separately for regular > and DA pool. It should be OK, but I feel there is a bug, which may, under > rare circumstances, lead to stall with my patch. It is probably a bug even > without the patch. > > Look, below, if it is time to activate DA worker, we call it explicitly. > But in this case we do not advise regular workers. They are likely already > running at this point, but it is not guaranteed. What if, for example, the > system is set to start additional workers when the queue is going over high > watermark? What if HighWatermark is set to 1? Regular workers will not be > started, and DA worker may fail. Thus, it is reasonable to advise regular > workers even if we are going DA. > > I have now reviewed it. I am also 99.9% sure it's a bug. I think I know it's root. Ages ago (v3?) we did not permit any direct queue workers when a DA worker was active. Other works were even deliberately shut down if they were active. This was when we tried to keep message sequence under all circumstances. Later, we realized that sequence can't be kept in any way, and so we relaxed that requirement -- and thus made regular workers and the DA worker run concurrently. I think, this was when the bug was introduced. I think it has not been discovered over the years as a) queue workers usually "just run" (and fail only if real bad things happen) and b) the queue even then seems operational as long as the DA worker continues to work (even though at considerably lower speed). Both conditions together were probably too-seldom to occur reproducible, if at all. Why am I saying this all? Because this is one reason that I think you patch is correct and there indeed is a bug, Makes sense in history context. Without history, it looks just plain right. So I'll now merge it, will be in the repro soon. Thanks again, Rainer _______________________________________________ rsyslog mailing list http://lists.adiscon.net/mailman/listinfo/rsyslog http://www.rsyslog.com/professional-services/ What's up with rsyslog? Follow https://twitter.com/rgerhards NOTE WELL: This is a PUBLIC mailing list, posts are ARCHIVED by a myriad of sites beyond our control. PLEASE UNSUBSCRIBE and DO NOT POST if you DON'T LIKE THAT.
