Skip to content

Refactor Connect/Disconnect out to CClient with an explicit connection state #3805

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
mcfnord wants to merge 4 commits into
base: main
Choose a base branch
from
+177 −98
Open

Refactor Connect/Disconnect out to CClient with an explicit connection state #3805

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
43b5eb5
Refactor Connect out to CClient
pgScorpio Sep 13, 2024
1b49a78
Add explicit client connection state machine
Jul 19, 2026
efee722
client: address review feedback on the connection-state refactor
Jul 21, 2026
a7c5c7b
client: restore OnAboutToQuit comment on exit() call
Jul 27, 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
87 changes: 81 additions & 6 deletions src/client.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ CClient::CClient ( const quint16 iPortNumber,
strClientName ( strNClientName ),
pSignalHandler ( CSignalHandler::getSingletonP() ),
pSettings ( nullptr ),
eConnectionState ( CS_DISCONNECTED ),
Channel ( false ), /* we need a client channel -> "false" */
CurOpusEncoder ( nullptr ),
CurOpusDecoder ( nullptr ),
Expand Down Expand Up @@ -145,7 +146,7 @@ CClient::CClient ( const quint16 iPortNumber,
// The first ConClientListMesReceived handler performs the necessary cleanup and has to run first:
QObject::connect ( &Channel, &CChannel::ConClientListMesReceived, this, &CClient::OnConClientListMesReceived );

QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Disconnected );
QObject::connect ( &Channel, &CChannel::Disconnected, this, &CClient::Stop );

QObject::connect ( &Channel, &CChannel::NewConnection, this, &CClient::OnNewConnection );

Expand Down Expand Up @@ -618,6 +619,9 @@ bool CClient::SetServerAddr ( QString strNAddr )
// 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
Expand Down Expand Up @@ -905,11 +909,8 @@ void CClient::OnHandledSignal ( int sigNum )
{
case SIGINT:
case SIGTERM:
// if connected, terminate connection (needed for headless mode)
if ( IsRunning() )
{
Stop();
}
// if connected, Stop client (needed for headless mode)
Stop();

// this should trigger OnAboutToQuit
Comment thread
pljones marked this conversation as resolved.
QCoreApplication::instance()->exit();
Expand Down Expand Up @@ -1003,6 +1004,9 @@ void CClient::OnClientIDReceived ( int iServerChanID )
SetRemoteChanGain ( iChanID, 0, false );
}

// the server has assigned us a channel ID, so the connection is established
SetConnectionState ( CS_CONNECTED );

emit ClientIDReceived ( iChanID );
}

Expand Down Expand Up @@ -1045,8 +1049,19 @@ void CClient::Start()
// Disable hibernation or display dimming if the app is running on Windows
SetThreadExecutionState ( ES_CONTINUOUS | ES_SYSTEM_REQUIRED | ES_DISPLAY_REQUIRED );
#endif

// the connection is requested now but not yet established: the transition
// to CS_CONNECTED happens when the server assigns our channel ID
// (see OnClientIDReceived)
SetConnectionState ( CS_CONNECTING );

emit Connecting ( GetConnectedServerName() );
}

/// @method
/// @brief Stops client and disconnects from server
/// @emit Disconnected
/// Use to set CClientDlg to show not being connected
void CClient::Stop()
{
// stop audio interface

@ann0see ann0see Jul 19, 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.

The IsRunning() guard is not needed? (don't remember the initial argument of my PR.)

@ann0see ann0see Jul 19, 2026 •
edited
Loading

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.

Please check this - especially for the sigterm case: https://github.com/jamulussoftware/jamulus/pull/3805/changes#diff-7bc46552432fa03e009f9f455e4729312853d92775935ef20849797f24ac0c60R912-R913

@mcfnord mcfnord Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed together with the guard above in efee722 — OnHandledSignal now calls Disconnect(), and the state-based guard means SIGTERM while CS_CONNECTING still sends CLM_DISCONNECTION (verified by capture).

@mcfnord mcfnord Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — a guard is needed, but IsRunning() was the wrong predicate.

IsRunning() is Sound.IsRunning() (the audio device). Start() sets CS_CONNECTING and calls Sound.Start() as separate steps, so the two diverge: during CS_CONNECTING, and in headless mode where audio may lag or be unavailable, the state is connecting/connected while IsRunning() is still false. Stop() is the only thing that sends CLDisconnection, and on the client there is no aboutToQuit handler and ~CClient() only does Sound.Stop() — so OnHandledSignal was calling raw Stop() precisely to dodge the guard and still notify the server on shutdown.

Fixed in efee722 by keying the guard off the explicit state this PR introduces, and routing SIGTERM through Disconnect():

void CClient::Disconnect()
{
    if ( GetConnectionState() != CS_DISCONNECTED ) { Stop(); }
}
// OnHandledSignal now: Disconnect();  (was: raw Stop();)

So Disconnect() stays idempotent (the repeated UI calls remain no-ops) and SIGTERM notifies the server whenever a connection is pending or established — including mid-connect and in headless mode. Dropped the stale // needed for headless mode / // this should trigger OnAboutToQuit comments (there is no client OnAboutToQuit).

Verified with a UDP capture: SIGTERM while connected → server receives CLM_DISCONNECTION; SIGTERM while idle → no spurious disconnect (the old raw Stop() sent one to a stale address); clean exit 0 in both cases.

Expand Down Expand Up @@ -1088,6 +1103,66 @@ void CClient::Stop()
// Allow hibernation or display dimming if the app is running again (Windows)
SetThreadExecutionState ( ES_CONTINUOUS );
#endif

SetConnectionState ( CS_DISCONNECTED );

// emit Disconnected() to inform UI of disconnection
emit Disconnected();
}

/// @method
/// @brief Stops the client if the client is running
/// @emit Disconnected
void CClient::Disconnect()
{
if ( IsRunning() )
{
Stop();
}
}

/// @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().
/// Use to set CClientDlg to show being connected
/// @emit ConnectingFailed (error) if an error occurred
/// Use to display error message in CClientDlg
/// @param strServerAddress - the server address to connect to
/// @param strServerName - the human readable server name passed to Connecting()
void CClient::Connect ( QString strServerAddress, QString strServerName )
{
try
{
// disconnect from any current server first so that connecting to a
// different server while connected behaves as a reconnect
Disconnect();

// Set server address and connect if valid address was supplied
if ( SetServerAddr ( strServerAddress ) )
{
SetConnectedServerName ( strServerName );
Start();
}
else
{
throw CGenErr ( tr ( "Received invalid server address. Please check for typos in the provided server address." ) );
}
}
catch ( const CGenErr& generr )
{
Stop();
emit ConnectingFailed ( generr.GetErrorText() );
}
}

void CClient::SetConnectionState ( const EConnectionState eNewConnectionState )

@ann0see ann0see Jul 19, 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.

Add documentation like for connect/disconnect.

@mcfnord mcfnord Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in efee722 — added a /// @method doc-comment.

{
if ( eConnectionState != eNewConnectionState )
{
eConnectionState = eNewConnectionState;
emit ConnectionStateChanged ( eConnectionState );
}
}

void CClient::Init()
Expand Down
24 changes: 23 additions & 1 deletion src/client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,15 @@ class CClient : public QObject

void Start();
void Stop();
void Disconnect();
void Connect ( QString strServerAddress, QString strServerName );

// The ConnectedServerName is emitted by Connecting() to update the UI with a human readable server name
void SetConnectedServerName ( const QString strServerName ) { strConnectedServerName = strServerName; };
QString GetConnectedServerName() const { return strConnectedServerName; };

EConnectionState GetConnectionState() const { return eConnectionState; }

bool IsRunning() { return Sound.IsRunning(); }
bool IsCallbackEntered() const { return Sound.IsCallbackEntered(); }
bool SetServerAddr ( QString strNAddr );
Expand Down Expand Up @@ -353,6 +362,14 @@ class CClient : public QObject
void FreeClientChannel ( const int iServerChannelID );
int FindClientChannel ( const int iServerChannelID, const bool bCreateIfNew ); // returns a client channel ID or INVALID_INDEX
bool ReorderLevelList ( CVector<uint16_t>& vecLevelList ); // modifies vecLevelList, passed by reference
// information for the connected server

@ann0see ann0see Jul 19, 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.

Which kind of information? Comment is unclear.

@mcfnord mcfnord Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in efee722 — reworded to "human-readable name of the current server, shown in the UI".

QString strConnectedServerName;

// single source of truth for the connection state, only to be modified
// via SetConnectionState() so that every change emits ConnectionStateChanged()

@ann0see ann0see Jul 19, 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.

Does not really add much and smells to much AI ish.

@mcfnord mcfnord Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in efee722 — trimmed to "current connection state; set only via SetConnectionState()".

EConnectionState eConnectionState;

void SetConnectionState ( const EConnectionState eNewConnectionState );

// only one channel is needed for client application
CChannel Channel;
Expand Down Expand Up @@ -464,7 +481,8 @@ protected slots:
{
if ( InetAddr == Channel.GetAddress() )
{
emit Disconnected();
// Stop client in case it received a Disconnection request
Stop();
}
}
void OnCLPingReceived ( CHostAddress InetAddr, int iMs );
Expand Down Expand Up @@ -507,7 +525,11 @@ protected slots:

void CLChannelLevelListReceived ( CHostAddress InetAddr, CVector<uint16_t> vecLevelList );

void ConnectionStateChanged ( EConnectionState eConnectionState );
void Connecting ( QString strServerName );
void ConnectingFailed ( QString errorMessage );
void Disconnected();

void SoundDeviceChanged ( QString strError );
void ControllerInFaderLevel ( int iChannelIdx, int iValue );
void ControllerInPanValue ( int iChannelIdx, int iValue );
Expand Down
119 changes: 39 additions & 80 deletions src/clientdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,6 @@
/* Implementation *************************************************************/
CClientDlg::CClientDlg ( CClient* pNCliP,
CClientSettings* pNSetP,
const QString& strConnOnStartupAddress,
const bool bNewShowComplRegConnList,
const bool bShowAnalyzerConsole,
const bool bMuteStream,
Expand Down Expand Up @@ -292,14 +291,6 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
TimerCheckAudioDeviceOk.setSingleShot ( true ); // only check once after connection
TimerDetectFeedback.setSingleShot ( true );

// Connect on startup ------------------------------------------------------
if ( !strConnOnStartupAddress.isEmpty() )
{
// initiate connection (always show the address in the mixer board
// (no alias))
Connect ( strConnOnStartupAddress, strConnOnStartupAddress );
}

// File menu --------------------------------------------------------------
QMenu* pFileMenu = new QMenu ( tr ( "&File" ), this );

Expand Down Expand Up @@ -507,7 +498,11 @@ CClientDlg::CClientDlg ( CClient* pNCliP,
// other
QObject::connect ( pClient, &CClient::ConClientListMesReceived, this, &CClientDlg::OnConClientListMesReceived );

QObject::connect ( pClient, &CClient::Disconnected, this, &CClientDlg::OnDisconnected );
QObject::connect ( pClient, &CClient::Connecting, this, &CClientDlg::OnConnecting );

QObject::connect ( pClient, &CClient::ConnectingFailed, this, &CClientDlg::OnConnectingFailed );

QObject::connect ( pClient, &CClient::Disconnected, this, &CClientDlg::OnDisconnect );

QObject::connect ( pClient, &CClient::ChatTextReceived, this, &CClientDlg::OnChatTextReceived );

Expand Down Expand Up @@ -646,11 +641,8 @@ void CClientDlg::closeEvent ( QCloseEvent* Event )
ConnectDlg.close();
AnalyzerConsole.close();

// if connected, terminate connection
if ( pClient->IsRunning() )
{
pClient->Stop();
}
// Disconnect if needed
pClient->Disconnect();

// make sure all current fader settings are applied to the settings struct
MainMixerBoard->StoreAllFaderSettings();
Expand Down Expand Up @@ -768,15 +760,8 @@ void CClientDlg::OnConnectDlgAccepted()
}
}

// first check if we are already connected, if this is the case we have to
// disconnect the old server first
if ( pClient->IsRunning() )
{
Disconnect();
}

// initiate connection
Connect ( strSelectedAddress, strMixerBoardLabel );
// initiate connection (terminates any current connection first)
pClient->Connect ( strSelectedAddress, strMixerBoardLabel );

// reset flag
bConnectDlgWasShown = false;
Expand All @@ -788,11 +773,12 @@ void CClientDlg::OnConnectDisconBut()
// the connect/disconnect button implements a toggle functionality
if ( pClient->IsRunning() )
{
Disconnect();
SetMixerBoardDeco ( RS_UNDEFINED, pClient->GetGUIDesign() );
pClient->Disconnect();
}
else
{
// If the client isn't running, we assume that we weren't connected. Thus show the connect dialog
// TODO: Refactor to have robust error handling
ShowConnectionSetupDialog();
}
}
Expand Down Expand Up @@ -904,7 +890,7 @@ void CClientDlg::OnLicenceRequired ( ELicenceType eLicenceType )
// disconnect from that server.
if ( !LicenceDlg.exec() )
{
Disconnect();
pClient->Disconnect();
}

// unmute the client output stream if local mute button is not pressed
Expand Down Expand Up @@ -1205,10 +1191,7 @@ void CClientDlg::OnSoundDeviceChanged ( QString strError )
if ( !strError.isEmpty() )
{
// the sound device setup has a problem, disconnect any active connection
if ( pClient->IsRunning() )
{
Disconnect();
}
pClient->Disconnect();

// show the error message of the device setup
QMessageBox::critical ( this, APP_NAME, strError, tr ( "Ok" ), nullptr );
Expand Down Expand Up @@ -1236,65 +1219,38 @@ void CClientDlg::OnCLPingTimeWithNumClientsReceived ( CHostAddress InetAddr, int
ConnectDlg.SetPingTimeAndNumClientsResult ( InetAddr, iPingTime, iNumClients );
}

void CClientDlg::Connect ( const QString& strSelectedAddress, const QString& strMixerBoardLabel )
void CClientDlg::OnConnecting ( const QString& strMixerBoardLabel )
{
// set address and check if address is valid
if ( pClient->SetServerAddr ( strSelectedAddress ) )
{
// try to start client, if error occurred, do not go in
// running state but show error message
try
{
if ( !pClient->IsRunning() )
{
pClient->Start();
}
}

catch ( const CGenErr& generr )
{
// show error message and return the function
QMessageBox::critical ( this, APP_NAME, generr.GetErrorText(), "Close", nullptr );
return;
}

// hide label connect to server
lblConnectToServer->hide();
lbrInputLevelL->setEnabled ( true );
lbrInputLevelR->setEnabled ( true );
// hide label connect to server
lblConnectToServer->hide();
lbrInputLevelL->setEnabled ( true );
lbrInputLevelR->setEnabled ( true );

// change connect button text to "disconnect"
butConnect->setText ( tr ( "&Disconnect" ) );
// change connect button text to "disconnect"
butConnect->setText ( tr ( "&Disconnect" ) );

// set server name in audio mixer group box title
MainMixerBoard->SetServerName ( strMixerBoardLabel );
// set server name in audio mixer group box title
MainMixerBoard->SetServerName ( strMixerBoardLabel );

// start timer for level meter bar and ping time measurement
TimerSigMet.start ( LEVELMETER_UPDATE_TIME_MS );
TimerBuffersLED.start ( BUFFER_LED_UPDATE_TIME_MS );
TimerPing.start ( PING_UPDATE_TIME_MS );
TimerCheckAudioDeviceOk.start ( CHECK_AUDIO_DEV_OK_TIME_MS ); // is single shot timer
// start timer for level meter bar and ping time measurement
TimerSigMet.start ( LEVELMETER_UPDATE_TIME_MS );
TimerBuffersLED.start ( BUFFER_LED_UPDATE_TIME_MS );
TimerPing.start ( PING_UPDATE_TIME_MS );
TimerCheckAudioDeviceOk.start ( CHECK_AUDIO_DEV_OK_TIME_MS ); // is single shot timer

// audio feedback detection
if ( pSettings->bEnableFeedbackDetection )
{
TimerDetectFeedback.start ( DETECT_FEEDBACK_TIME_MS ); // single shot timer
bDetectFeedback = true;
}
// audio feedback detection
if ( pSettings->bEnableFeedbackDetection )
{
TimerDetectFeedback.start ( DETECT_FEEDBACK_TIME_MS ); // single shot timer
bDetectFeedback = true;

@ann0see ann0see Jul 19, 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 yes, this should also not be in the UI.

@mcfnord mcfnord Jul 21, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed, out of scope for this PR — left as-is for a separate change.

}
}

void CClientDlg::Disconnect()
{
// only stop client if currently running, in case we received
// the stopped message, the client is already stopped but the
// connect/disconnect button and other GUI controls must be
// updated
if ( pClient->IsRunning() )
{
pClient->Stop();
}
void CClientDlg::OnConnectingFailed ( const QString& strError ) { QMessageBox::critical ( this, APP_NAME, strError, tr ( "Close" ), nullptr ); }

void CClientDlg::OnDisconnect()
{
// change connect button text to "connect"
butConnect->setText ( tr ( "C&onnect" ) );

Expand Down Expand Up @@ -1336,6 +1292,9 @@ void CClientDlg::Disconnect()

// clear mixer board (remove all faders)
MainMixerBoard->HideAll();

// Reset the deco
SetMixerBoardDeco ( RS_UNDEFINED, pClient->GetGUIDesign() );
}

void CClientDlg::UpdateDisplay()
Expand Down
Loading
Loading