[PATCH] messaging_service: do not call uninitialized _address_to_host_id_mapper std::function

[Gleb Natapov]

During messaging_service object creation remove_rpc_client function may
be called if prefer_local snitch setting is true. The caller does not
provide host id, so _address_to_host_id_mapper is called to obtain it,
but at this point the function is not initialized yet.

The patch fixes the code to not call the function if not initialized.
This is not the problem since during messaging_service creation there
is no connection to drop.

Fixed: #23353

---
CI: https://jenkins.scylladb.com/job/scylla-master/job/scylla-ci/16323/

diff --git a/test/cluster/test_multidc.py b/test/cluster/test_multidc.py
index c2a003d51f5..e7592b1e8ee 100644
--- a/test/cluster/test_multidc.py
+++ b/test/cluster/test_multidc.py
@@ -462,3 +462,16 @@ async def test_startup_with_keyspaces_violating_rf_rack_valid_keyspaces(manager:
await try_fail([4, 1], "dc1", 4, 3)

_ = await manager.server_start(s1.server_id)
+
+...@pytest.mark.asyncio
+async def test_restart_with_prefer_local(request: pytest.FixtureRequest, manager: ManagerClient) -> None:
+ logger.info("Creating a new cluster")
+ for i in range(3):
+ s_info = await manager.server_add(
+ config=CONFIG,
+ property_file={'dc': f'dc{i}', 'rack': 'myrack1', 'prefer_local': 'true'}
+ )
+ logger.info(s_info)
+
+ await manager.server_stop_gracefully(s_info.server_id)
+ await manager.server_start(s_info.server_id)
diff --git a/message/messaging_service.cc b/message/messaging_service.cc
index 0c2da49a4a8..65ad830134a 100644
--- a/message/messaging_service.cc
+++ b/message/messaging_service.cc
@@ -1224,6 +1224,12 @@ void messaging_service::remove_rpc_client(msg_addr id, std::optional<locator::ho
find_and_remove_client(c, id, [] (const auto&) { return true; });
}
if (!hid) {
+ if (!_address_to_host_id_mapper) {
+ if (!_clients_with_host_id.empty()) {
+ mlogger.warn("remove_rpc_client is called without host id and mapper function is not initialized yet");
+ }
+ return;
+ }
hid = _address_to_host_id_mapper(id.addr);
}
for (auto& c : _clients_with_host_id) {
--
Gleb.

[Avi Kivity]

This warning is complaining about something that isn't broken and there is nothing that the user can do to fix it.

[Gleb Natapov]

This should not happen (non empty client list and unset
_address_to_host_id_mapper), so it is broken if it happens. Internal
error that throws is too harsh here.

>
> >          hid = _address_to_host_id_mapper(id.addr);
> >      }
> >      for (auto& c : _clients_with_host_id) {
> > --
> > Gleb.
> >
>

--
Gleb.

[Gleb Natapov]

Ping. This fixes pretty serious bug (nodes cannot come up after reboot
forever) so we need to resolve it ASAP.

--
Gleb.

[Avi Kivity]

If it doesn't happen, how can it fix a serious bug?

Obviously you expect it to happen.

Ah, the nested if doesn't happen, but the outer if does.