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 1 commit
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.

This is legacy code which would crash with assertion on shutdown when the ICP incoming and outgoing ports were the same.

This PR uses our current code style (used for other similar ports), which prevents these assertions from occuring by closing the out-port when it is open and leaving it as a reference to a closed port if it is a duplicate of the in-port.

  • on shutdown any hanging reference will be dropped by the system.
  • on reconfigure the reference will be unconditionally replaced with a new open one.
  • we can be extra paranoid and move icpOutgoingConn = nullptr; outside the new if-statement to free the memory early as-needed.


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 startShutdown() 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
Loading