Skip to content

Manage ICP ports using a Runner #2365

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
yadij wants to merge 2 commits into
base: master
Choose a base branch
from
+22 −27
Open

Manage ICP ports using a Runner #2365

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a138df2
Manage ICP ports using a Runner
yadij Jan 31, 2026
c9384b4
Reviewer requested change
yadij Feb 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/ICP.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,10 +114,8 @@ void icpHandleIcpV3(int, Ip::Address &, char *, int);
void icpOpenPorts(void);

/// \ingroup ServerProtocolICPAPI
void icpConnectionShutdown(void);

/// \ingroup ServerProtocolICPAPI
void icpClosePorts(void);
/// Perform a graceful shutdown of ICP listening and sending ports (if any)
Copy link
Contributor

@rousskov rousskov Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me what "graceful shutdown" implies in this context. Moreover, I do not think this function actually shuts down any ports -- the ports may continue to be open after this function returns. If fixing existing icpClosePorts() problems is outside this PR scope, then I recommend not adding any description of this problematic function in this PR.

Copy link
Contributor Author

@yadij yadij Feb 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not clear to me what "graceful shutdown" implies in this context. Moreover, I do not think this function actually shuts down any ports -- the ports may continue to be open after this function returns.

In other words; the global port references are set to "closed" (for new activity), while the ports themselves are left to be closed "gracefully" (via RAII) when the last active transaction using them is done.

void icpClosePorts();

/// \ingroup ServerProtocolICPAPI
int icpSetCacheKey(const cache_key * key);
Expand Down
34 changes: 19 additions & 15 deletions src/icp_v2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "acl/Acl.h"
#include "acl/FilledChecklist.h"
#include "base/AsyncCallbacks.h"
#include "base/RunnersRegistry.h"
#include "client_db.h"
#include "comm.h"
#include "comm/Connection.h"
Expand Down Expand Up @@ -677,6 +678,9 @@ icpHandleUdp(int sock, void *)
void
icpOpenPorts(void)
{
if (IamWorkerProcess())
return;

uint16_t port;

if ((port = Config.Port.icp) <= 0)
Expand Down Expand Up @@ -752,13 +756,12 @@ icpIncomingConnectionOpened(Ipc::StartListeningAnswer &answer)
}
}

/**
* icpConnectionShutdown only closes the 'in' socket if it is
* different than the 'out' socket.
*/
void
icpConnectionShutdown(void)
icpClosePorts()
{
if (!IamWorkerProcess())
return;

if (!Comm::IsConnOpen(icpIncomingConn))
return;

Expand All @@ -776,22 +779,23 @@ icpConnectionShutdown(void)
* to that specific interface. During shutdown, we must
* disable reading on the outgoing socket.
*/
assert(Comm::IsConnOpen(icpOutgoingConn));
Copy link
Contributor

@rousskov rousskov Feb 2, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, the removed assert checked an invariant that this function still uses. The assert did not check everything it should have checked, but do we have to remove it in this PR?

Copy link
Contributor Author

@yadij yadij Feb 7, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The invariant I see here is that the outgoing port is already closed. Either by being the same object used by incoming port, or having closed already (eg by that read handler getting a 0-byte read).

What else do you think also needs to be checked in order to close the outgoing port?
given that this logic is now being run on starting shutdown/reconfigure.


Comm::SetSelect(icpOutgoingConn->fd, COMM_SELECT_READ, nullptr, nullptr, 0);
}

void
icpClosePorts(void)
{
icpConnectionShutdown();
if (Comm::IsConnOpen(icpOutgoingConn)) {
Comm::SetSelect(icpOutgoingConn->fd, COMM_SELECT_READ, nullptr, nullptr, 0);

if (icpOutgoingConn != nullptr) {
debugs(12, DBG_IMPORTANT, "Stop sending ICP from " << icpOutgoingConn->local);
icpOutgoingConn = nullptr;
}
}

class IcpRr : public RegisteredRunner
{
void useConfig() override { icpOpenPorts(); }
void startReconfigure() override { icpClosePorts(); }
void syncConfig() override { icpOpenPorts(); }
void endingShutdown() override { icpClosePorts(); }
};
DefineRunnerRegistrator(IcpRr);

static void
icpCount(void *buf, int which, size_t len, int delay)
{
Expand Down
5 changes: 1 addition & 4 deletions src/main.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,6 @@ serverConnectionsOpen(void)
// start various proxying services if we are responsible for them
if (IamWorkerProcess()) {
clientOpenListenSockets();
icpOpenPorts();
icmpEngine.Open();
netdbInit();
Acl::Node::Initialize();
Expand All @@ -791,7 +790,6 @@ serverConnectionsClose(void)

if (IamWorkerProcess()) {
clientConnectionsClose();
icpConnectionShutdown();
icmpEngine.Close();
}
}
Expand All @@ -809,7 +807,6 @@ mainReconfigureStart(void)

// Initiate asynchronous closing sequence
serverConnectionsClose();
icpClosePorts();
#if USE_OPENSSL
Ssl::TheGlobalContextStorage().reconfigureStart();
#endif
Expand Down Expand Up @@ -1396,6 +1393,7 @@ RegisterModules()
#if USE_HTCP
CallRunnerRegistrator(HtcpRr);
#endif
CallRunnerRegistrator(IcpRr);
#if USE_OPENSSL
CallRunnerRegistrator(sslBumpCfgRr);
#endif
Expand Down Expand Up @@ -2015,7 +2013,6 @@ SquidShutdown()
#endif
redirectShutdown();
externalAclShutdown();
icpClosePorts();
releaseServerSockets();
commCloseAllSockets();

Expand Down
1 change: 0 additions & 1 deletion src/tests/stub_icp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ void icpCreateAndSend(icp_opcode, int, char const *, int, int, int, const Ip::Ad
icp_opcode icpGetCommonOpcode() STUB_RETVAL(ICP_INVALID)
void icpDenyAccess(Ip::Address &, char *, int, int) STUB
void icpHandleIcpV3(int, Ip::Address &, char *, int) STUB
void icpConnectionShutdown(void) STUB
int icpSetCacheKey(const cache_key *) STUB_RETVAL(0)
const cache_key *icpGetCacheKey(const char *, int) STUB_RETVAL(nullptr)

Expand Down
3 changes: 0 additions & 3 deletions src/tools.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,6 @@ releaseServerSockets(void)
// clear http_port, https_port, and ftp_port lists
clientConnectionsClose();

// clear icp_port's
icpClosePorts();

// XXX: Why not the HTCP, SNMP, DNS ports as well?
// XXX: why does this differ from main closeServerConnections() anyway ?
}
Expand Down
Loading