Skip to content

Convert host and SRV lookups to async instead of blocking #3986

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

Draft
softins wants to merge 5 commits into
base: main
Choose a base branch
from
Draft

Convert host and SRV lookups to async instead of blocking #3986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4bda78d
Convert host and SRV lookups to async instead of blocking
softins Nov 9, 2026
c3e897a
Fix typo
softins Nov 9, 2026
4c42f34
Remove unused #define
softins Nov 9, 2026
18fbfa1
Add sequence number for CClient Connect and Disconnect
softins Nov 10, 2026
156cfed
Add sequence number for CConnectDlg directory resolution
softins Nov 12, 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
82 changes: 55 additions & 27 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ CClient::CClient ( const quint16 iPortNumber,
bJitterBufferOK ( true ),
bMuteMeInPersonalMix ( bNMuteMeInPersonalMix ),
iServerSockBufNumFrames ( DEF_NET_BUF_SIZE_NUM_BL ),
bRawAudioIsSupported ( false )
bRawAudioIsSupported ( false ),
iResolverSeq ( 0 )
{
int iOpusError;

Expand Down Expand Up @@ -619,25 +620,6 @@ void CClient::SetRemoteChanPan ( const int iId, const float fPan )
StartTimerGainOrPan();
}

bool CClient::SetServerAddr ( QString strNAddr )
{
CHostAddress HostAddress;
if ( NetworkUtil::ParseNetworkAddress ( strNAddr, HostAddress, bIPv6Available ) )
{
// apply address to the channel
Channel.SetAddress ( HostAddress );

// By default, set server name to HostAddress. If using the Connect() method, this may be overwritten
SetConnectedServerName ( HostAddress.toString() );

return true;
}
else
{
return false; // invalid address
}
}

bool CClient::GetAndResetbJitterBufferOKFlag()
{
// get the socket buffer put status flag and reset it
Expand Down Expand Up @@ -1150,6 +1132,8 @@ void CClient::Stop()
/// @emit Disconnected
void CClient::Disconnect()
{
++iResolverSeq; // invalidate any uncompleted resolver requests

// Key off the connection state, not IsRunning() (which tracks the audio
// device): the two diverge while connecting and in headless mode, and on
// SIGTERM we must still send the disconnect message to the server.
Expand All @@ -1162,7 +1146,7 @@ void CClient::Disconnect()
/// @method
/// @brief Connects to strServerAddress. If a connection is currently requested
/// or established, that connection is terminated first.
/// @emit Connecting (strServerName) if SetServerAddr was valid. emit happens through Start().
/// @emit Connecting (strServerName) if address was valid. emit happens through Start().
/// Use to set CClientDlg to show being connected
/// @emit ConnectingFailed (error) if an error occurred
/// Use to display error message in CClientDlg
Expand All @@ -1176,24 +1160,68 @@ void CClient::Connect ( const QString& strServerAddress, const QString& strServe
// different server while connected behaves as a reconnect
Disconnect();

// Set server address and connect if valid address was supplied
if ( SetServerAddr ( strServerAddress ) )
const int seq = iResolverSeq; // for capturing the current request

if ( NetworkUtil::ResolveAddress (
strServerAddress,
bIPv6Available,
this,
[this, seq] ( const QHostInfo& info, quint16 port ) {
// check that the request has not been invalidated
if ( seq == iResolverSeq )
{
OnHostResolved ( info, port ); // handle result
}
},
true,
true ) )
{
SetConnectedServerName ( strServerName );
Start();
SetConnectedServerName ( strServerName ); // set if resolution initiated ok
}
else
{
throw CGenErr ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) );
emit ConnectingFailed ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) );
}
}
catch ( const CGenErr& generr )
{
Stop();
// Stop(); // XXX: probably not needed?

@ann0see ann0see Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

TODO: investigate what errors can occur and then if we can get into an inconsistent state.

CC: @coderabbitai

@coderabbitai coderabbitai Bot Nov 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Analysis chain

Script executed:

#!/bin/bash
set -u
echo '--- repository revision ---'
git rev-parse HEAD
echo '--- relevant outlines ---'
ast-grep outline src/util.cpp --match 'ResolveAddress|parseHostAndPort|validateHostnameFormat' --view expanded
ast-grep outline src/client.cpp --match 'Connect|Disconnect|OnHostResolved|Stop|Start' --view expanded
ast-grep outline src/client.h --match 'SetConnectedServerName|GetConnectionState|Connect|Disconnect' --view expanded
echo '--- resolver and client operations ---'
rg -n -C 3 'ResolveAddress\s*\(|lookupHost\s*\(|QDnsLookup|singleShot\s*\(' src/util.cpp src/util.h
rg -n -C 3 'SetConnectedServerName\s*\(|ConnectingFailed|\.Connect\s*\(|->Connect\s*\(|\.Disconnect\s*\(|->Disconnect\s*\(' src
echo '--- sound backend error locations ---'
rg -n -C 2 'throw\s+CGenErr|void\s+CSound.*::Stop|void\s+CSound.*::Start|int\s+CSound.*::Init' src -g '*sound*' -g '*Sound*' | head -300

Length of output: 23425

Script executed:

#!/bin/bash
set -u
echo '--- resolver and callback adapter ---'
cat -n src/util.cpp | sed -n '855,865p;867,1130p'
cat -n src/util.h | sed -n '1100,1145p'
echo '--- Oboe backend outline and relevant code ---'
ast-grep outline src/sound/oboe/sound.cpp --match 'closeStreams|openStreams|Start|Stop|Init' --view expanded
cat -n src/sound/oboe/sound.cpp | sed -n '45,210p'
echo '--- JACK and base sound backend ---'
ast-grep outline src/sound/jack/sound.cpp --match 'Start|Stop|Init' --view expanded
ast-grep outline src/sound/soundbase.cpp --match 'Start|Stop|SetError' --view expanded
cat -n src/sound/jack/sound.cpp | sed -n '185,205p;320,350p'
cat -n src/sound/soundbase.cpp | sed -n '35,85p;150,200p'
echo '--- gui connection handling ---'
ast-grep outline src/clientdlg.cpp --match 'OnConnecting|OnConnectingFailed|OnDisconnect|OnConnectDisconBut' --view expanded
cat -n src/clientdlg.cpp | sed -n '750,810p;1220,1305p'
echo '--- server name and relevant state ---'
cat -n src/client.h | sed -n '90,103p;160,195p;370,390p'
cat -n src/client.cpp | sed -n '1038,1208p'

Length of output: 47363

@ann0see I found a CGenErr path that can leave the client in an inconsistent state.

  • During Connect(): Disconnect() calls Stop() when a connection is active. On Android, CSound::Stop() can throw if Oboe fails to stop or close a stream (src/sound/oboe/sound.cpp, Line 79). In that case, CClient::Stop() exits before it disables the channel, sends the disconnect message, or updates the connection state. The Connect() catch reports the error but does not complete cleanup.
  • During resolution: Invalid input returns false. DNS failure reaches OnHostResolved() through QHostInfo. Neither is a CGenErr handled by the outer catch.
  • During startup: Start() runs in OnHostResolved(), so its CGenErr is handled by the callback, not the outer catch. The callback calls Stop(), but that call can also throw before ConnectingFailed is emitted.

Restoring the unconditional Stop() in the outer catch is not sufficient: it can repeat the same shutdown failure. Cleanup should handle sound-stop errors while still attempting the remaining disconnect steps.

There is also a separate asynchronous state risk. A lookup remains pending while the state is CS_DISCONNECTED, so Disconnect() does not cancel it. An older result can start a connection after a newer request or cancellation. A result can also arrive while Stop() processes events. I recommend invalidating pending attempts before disconnecting and ignoring callbacks from older attempts.

Please test an Oboe stop failure, a startup failure, and delayed lookups followed by disconnect or a second connection request. This assessment is based on code inspection; I did not run the application or tests.

You are interacting with an AI system.

emit ConnectingFailed ( generr.GetErrorText() );
}
}

void CClient::OnHostResolved ( const QHostInfo& info, quint16 port )
{
if ( info.error() != QHostInfo::NoError )
{
emit ConnectingFailed ( "Cannot connect: " + info.errorString() );

@ann0see ann0see Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might want to have a tr() to allow translation?

return;
}

for ( const QHostAddress& HostAddr : info.addresses() )
{
// qInfo() << "Resolved network address to" << HostAddr << "for proto" << HostAddr.protocol();
if ( HostAddr.protocol() == QAbstractSocket::IPv4Protocol || ( HostAddr.protocol() == QAbstractSocket::IPv6Protocol && bIPv6Available ) )
Comment thread
ann0see marked this conversation as resolved.
Resolved
{
CHostAddress HostAddress ( HostAddr, port );
Channel.SetAddress ( HostAddress );
// sound device start can throw an exception, which we must catch here
try
{
Start();
}
catch ( const CGenErr& generr )

@ann0see ann0see Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not part of this PR, but in general error handling is not great in Jamulus at the moment. So we might want to think about how to handle this in future. This is probably fine-ish (but a CGenErr is probably too general from a theoretical PoV...)

{
Stop();
emit ConnectingFailed ( generr.GetErrorText() );
}
return;
}
}

emit ConnectingFailed ( "Hostname returned no usable addresses" );

@ann0see ann0see Nov 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

tr()? Moreover, I don't think this will tell the user enough. In general error messages should tell the user how they can fix the issue. I believe that we document in the style guide how error messages should look like. cc: @gilgongo ?

}

/// @method
/// @brief Updates the connection state and, if it changed, notifies observers.
/// @emit ConnectionStateChanged (state) when the state actually changes
Expand Down
5 changes: 4 additions & 1 deletion src/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,6 @@ class CClient : public QObject

bool IsRunning() { return Sound.IsRunning(); }
bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); }
bool SetServerAddr ( QString strNAddr );

// IPv6 Available
bool IsIPv6Available() { return bIPv6Available; }
Expand Down Expand Up @@ -478,6 +477,9 @@ class CClient : public QObject
int maxGainOrPanId;
int iCurPingTime;

// sequence number for asynchronous DNS resolver
int iResolverSeq;

protected slots:
void OnHandledSignal ( int sigNum );
void OnSendProtMessage ( CVector<uint8_t> vecMessage );
Expand Down Expand Up @@ -514,6 +516,7 @@ protected slots:
void OnMuteStateHasChangedReceived ( int iServerChanID, bool bIsMuted );
void OnCLChannelLevelListReceived ( CHostAddress InetAddr, CVector<uint16_t> vecLevelList );
void OnConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo );
void OnHostResolved ( const QHostInfo& info, quint16 port );

signals:
void ConClientListMesReceived ( CVector<CChannelInfo> vecChanInfo );
Expand Down
33 changes: 25 additions & 8 deletions src/clientdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,21 +605,38 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
// that the connection less message respond may not make it back but that
// is not critical since the next time Jamulus is started we have another
// chance and the update check is not time-critical at all)
CHostAddress UpdateServerHostAddress;

// Send the request to two servers for redundancy if either or both of them
// has a higher release version number, the reply will trigger the notification.

// Don't use SRV resolution when resolving update servers.
if ( !NetworkUtil::ResolveAddress ( UPDATECHECK1_ADDRESS, pClient->IsIPv6Available(), this, &CClientDlg::OnHostResolved, true, false ) )
{
qWarning() << "Cannot start update check for" << UPDATECHECK1_ADDRESS;
}

if ( !NetworkUtil::ResolveAddress ( UPDATECHECK2_ADDRESS, pClient->IsIPv6Available(), this, &CClientDlg::OnHostResolved, true, false ) )
{
qWarning() << "Cannot start update check for" << UPDATECHECK2_ADDRESS;
}
}

if ( NetworkUtil::ParseNetworkAddressBare ( UPDATECHECK1_ADDRESS, UpdateServerHostAddress, pClient->IsIPv6Available() ) )
void CClientDlg::OnHostResolved ( const QHostInfo& info, quint16 port )
{
if ( info.error() != QHostInfo::NoError )
{
pClient->CreateCLServerListReqVerAndOSMes ( UpdateServerHostAddress );
qWarning() << "Update lookup failed:" << info.errorString();
return;
}

if ( NetworkUtil::ParseNetworkAddressBare ( UPDATECHECK2_ADDRESS, UpdateServerHostAddress, pClient->IsIPv6Available() ) )
for ( const QHostAddress& HostAddr : info.addresses() )
{
pClient->CreateCLServerListReqVerAndOSMes ( UpdateServerHostAddress );
// qInfo() << "Resolved network address to" << HostAddr << "for proto" << HostAddr.protocol();
if ( HostAddr.protocol() == QAbstractSocket::IPv4Protocol ||
( HostAddr.protocol() == QAbstractSocket::IPv6Protocol && pClient->IsIPv6Available() ) )
{
CHostAddress HostAddress ( HostAddr, port );

pClient->CreateCLServerListReqVerAndOSMes ( HostAddress );
return;
}
}
}

Expand Down
1 change: 1 addition & 0 deletions src/clientdlg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ public slots:
void SetMixerBoardDeco ( const ERecorderState newRecorderState, const EGUIDesign eNewDesign );
void OnAudioChannelsChanged() { UpdateRevSelection(); }
void OnNumClientsChanged ( int iNewNumClients );
void OnHostResolved ( const QHostInfo& info, quint16 port );

void accept() { close(); } // introduced by pljones

Expand Down
36 changes: 30 additions & 6 deletions src/clientrpc.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,18 +187,42 @@ CClientRpc::CClientRpc ( CClient* pClient, CClientSettings* pSettings, CRpcServe
return;
}

CHostAddress haDirectoryAddress;

// Allow IPv4 only for communicating with Directories
if ( !NetworkUtil::ParseNetworkAddress ( jsonDirectoryIp.toString(), haDirectoryAddress, false ) )
// Start asynchronous address resolution with lambda callback slot
bool accepted = NetworkUtil::ResolveAddress (
jsonDirectoryIp.toString(),
false, // Currently IPv4 only for directories
pClient,
[pClient] ( const QHostInfo& info, quint16 port ) {
// this callback runs later, after the RPC handler has returned
if ( info.error() != QHostInfo::NoError )
{
qWarning() << "Directory lookup failed:" << info.errorString();
return;
}

// select a usable IPv4 address
for ( const QHostAddress& addr : info.addresses() )
{
if ( addr.protocol() == QAbstractSocket::IPv4Protocol )
{
CHostAddress haDirectoryAddress ( addr, port );

pClient->CreateCLReqServerListMes ( haDirectoryAddress );
return;
}
}
qWarning() << "No usable address found for directory";
},
true, // allow hostname lookup
true ); // allow SRV lookup

if ( !accepted )
{
response["error"] =
CRpcServer::CreateJsonRpcError ( CRpcServer::iErrInvalidParams, "Invalid params: directory is not a valid socket address" );
return;
}

// send the request for the server list
pClient->CreateCLReqServerListMes ( haDirectoryAddress );
response["result"] = "ok";
} );

Expand Down
74 changes: 57 additions & 17 deletions src/connectdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ CConnectDlg::CConnectDlg ( CClient* pNCliP, CClientSettings* pNSetP, const bool
bReducedServerListReceived ( false ),
bServerListItemWasChosen ( false ),
bListFilterWasActive ( false ),
bShowAllMusicians ( true )
bShowAllMusicians ( true ),
iResolverSeq ( 0 )
{
setupUi ( this );

Expand Down Expand Up @@ -312,24 +313,63 @@ void CConnectDlg::RequestServerList()
}
cbxDirectory->blockSignals ( false );

// Get the IP address of the directory server (using the ParseNetworkAddress
// Get the IP address of the directory server (using the async ResolveAddress
// function) when the connect dialog is opened, this seems to be the correct
// time to do it. Note that in case of custom directories we
// use iCustomDirectoryIndex as an index into the vector.

// Allow IPv4 only for communicating with Directories
if ( NetworkUtil::ParseNetworkAddress (
NetworkUtil::GetDirectoryAddress ( pSettings->eDirectoryType, pSettings->vstrDirectoryAddress[pSettings->iCustomDirectoryIndex] ),
haDirectoryAddress,
false ) )
// Currently allow only IPv4 for communicating with Directories

const QString strDirectoryAddress =
NetworkUtil::GetDirectoryAddress ( pSettings->eDirectoryType, pSettings->vstrDirectoryAddress[pSettings->iCustomDirectoryIndex] );

// get a new sequence number for matching the resolved callback
const int seq = ++iResolverSeq;

if ( !NetworkUtil::ResolveAddress (
strDirectoryAddress,
pClient->IsIPv6Available(),
this,
[this, seq] ( const QHostInfo& info, quint16 port ) {
// check that the request has not been superseded
if ( seq == iResolverSeq )
{
OnHostResolved ( info, port ); // handle result
}
},
true,
true ) )
{
// send the request for the server list
emit ReqServerListQuery ( haDirectoryAddress );
qWarning() << "Cannot start directory fetch from" << strDirectoryAddress;
}
}

void CConnectDlg::OnHostResolved ( const QHostInfo& info, quint16 port )
{
if ( info.error() != QHostInfo::NoError )
{
qWarning() << "Directory lookup failed:" << info.errorString();
return;
}

// start timer, if this message did not get any respond to retransmit
// the server list request message
TimerReRequestServList.start ( SERV_LIST_REQ_UPDATE_TIME_MS );
TimerInitialSort.start ( SERV_LIST_REQ_UPDATE_TIME_MS ); // reuse the time value
for ( const QHostAddress& HostAddr : info.addresses() )
{
// qInfo() << "Resolved network address to" << HostAddr << "for proto" << HostAddr.protocol();
if ( HostAddr.protocol() == QAbstractSocket::IPv4Protocol /* ||
( HostAddr.protocol() == QAbstractSocket::IPv6Protocol && pClient->IsIPv6Available() ) */ )
{
haDirectoryAddress = CHostAddress ( HostAddr, port );

// send the request for the server list
emit ReqServerListQuery ( haDirectoryAddress );

// start timer, if this message did not get any respond to retransmit
// the server list request message
TimerReRequestServList.start ( SERV_LIST_REQ_UPDATE_TIME_MS );
TimerInitialSort.start ( SERV_LIST_REQ_UPDATE_TIME_MS ); // reuse the time value

return;
}
}
}

Expand Down Expand Up @@ -855,10 +895,10 @@ void CConnectDlg::OnTimerPing()

// try to parse host address string which is stored as user data
// in the server list item GUI control element
// the data to be parsed is just IP:port, so no SRV discovery is needed
if ( NetworkUtil::ParseNetworkAddressBare ( pCurListViewItem->data ( LVC_NAME, Qt::UserRole ).toString(),
haServerAddress,
pClient->IsIPv6Available() ) )
// the data to be parsed is just IP:port, so no hostname lookup is needed
if ( NetworkUtil::ParseAddressLiteral ( pCurListViewItem->data ( LVC_NAME, Qt::UserRole ).toString(),
pClient->IsIPv6Available(),
haServerAddress ) )
{
// if address is valid, send ping message using a new thread
#if QT_VERSION >= QT_VERSION_CHECK( 6, 0, 0 )
Expand Down
4 changes: 4 additions & 0 deletions src/connectdlg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,9 @@ class CConnectDlg : public CBaseDlg, private Ui_CConnectDlgBase
bool bListFilterWasActive;
bool bShowAllMusicians;

// sequence number for asynchronous DNS resolver
int iResolverSeq;

public slots:
void OnServerListItemDoubleClicked ( QTreeWidgetItem* Item, int );
void OnDirectoryChanged ( int iTypeIdx );
Expand All @@ -157,6 +160,7 @@ public slots:
void OnTimerPing();
void OnTimerReRequestServList();
void OnCurrentServerItemChanged ( QTreeWidgetItem* current, QTreeWidgetItem* previous );
void OnHostResolved ( const QHostInfo& info, quint16 port );

signals:
void ReqServerListQuery ( CHostAddress InetAddr );
Expand Down
Loading
Loading