Fix major problems introduced by commit 1162165

[yadij]

... when Squid is built with select() or poll() I/O loops, and
on systems with limited resources.

[yadij]

FWIW, Now that this condition is using SQUID_MAXFD_LIMIT with a sane value - we can probably drop the USE_SELECT wrapper (and modify the debugs text) - which will in turn allow removal of the checkLimits variable.

[rousskov]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

[kinkie]

we can use the type system.
The change has been build tested

[yadij]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

This PR is implementing:

squid/src/tools.cc

Line 823 in dd5ea20

// XXX: When checkLimits, take ./configure --with-filedescriptors (if any) into account.

and fixing the issues documented alongside that:

1. squid/src/tools.cc

   Line 826 in dd5ea20

   // XXX: If this increases Squid_MaxFD, then the new value will violate any defined SQUID_MAXFD_LIMIT.

Squid select(2) is often enabled by ./configure as a backup I/O mechanism. As a result the Squid_MaxFD is by default almost always initialized to FD_SETSIZE - which is a relatively small value suitable for select(2) or poll(2) API structures.

2. squid/src/tools.cc

   Line 825 in dd5ea20

   // XXX: The new value may make Squid_MaxFD different from SQUID_MAXFD still used by ModEpoll, ModPoll, ipcCreate(), etc.!

This is actually guaranteed to occur for at least a period during Squid startup (from

squid/src/main.cc

Lines 1433 to 1434 in dd5ea20

	if (SQUID_MAXFD_LIMIT < Squid_MaxFD)
	Squid_MaxFD = SQUID_MAXFD_LIMIT;

until

squid/src/main.cc

Lines 1337 to 1339 in dd5ea20

	StartUsingConfig()
	{
	setMaxFD();

). Due to FD_SETSIZE being something smaller than the arbitrary 100*1024 constant.

[rousskov]

PR description: Fix major problems introduced by commit 1162165

What major problems does this PR fix?

This PR is implementing:

squid/src/tools.cc

Line 823 in dd5ea20

// XXX: When checkLimits, take ./configure --with-filedescriptors (if any) into account.

and fixing the issues documented alongside that:

1. squid/src/tools.cc

   Line 826 in dd5ea20

   // XXX: If this increases Squid_MaxFD, then the new value will violate any defined SQUID_MAXFD_LIMIT.

2. squid/src/tools.cc

   Line 825 in dd5ea20

   // XXX: The new value may make Squid_MaxFD different from SQUID_MAXFD still used by ModEpoll, ModPoll, ipcCreate(), etc.!

AFAICT, all of the above references point to problems that existed before commit 1162165. In other words, those problems were not "introduced by commit 1162165" as claimed in the PR title. The corresponding XXXs just documented reality that existed before that commit and continued to exist after that commit.

The discussion-relevant changes introduced by commit 1162165 can be summarized by this diff (which replaces rl.rlim_cur variable with its billion descriptors value in relevant Kubernets environments):

- Squid_MaxFD = 1073741816;
+ Squid_MaxFD =     102400;

Both values may be wrong in some cases (hence, XXXs), but the new/smaller value is certainly not worse than the old one in those problematic cases.

Even if you disagree with the above analysis, the current PR title/description does not disclose what "major problems" this PR is fixing. None of those XXXs can be easily identified as "major problems" IMO, and it is not clear that the current PR title is actually referring to those XXXs in the first place!

This PR direction was explicitly rejected during commit 1162165 (i.e. PR #2483) work as detailed in unofficial commit ce8c540 message. That fact does not imply that this direction is necessarily wrong -- different PRs have different scopes and evil comparison functions -- but I believe the original reasons for rejecting this direction are valid, and nothing in this PR convinces me that a different decision should be made now, in this PR.

I am pretty sure that, after the backlog issue is dealt with, the correct path forward starts with agreeing on what max_filedescriptors directive should represent (including the default behavior) and then adjusting code to match that agreement. I speculate that, on that path, we may "remove SQUID_MAXFD and its guessing ./configure code that leads to many inconsistencies and complications" (as was suggested in ce8c540).