-
Notifications
You must be signed in to change notification settings - Fork 245
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.
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
3
commits into
jamulussoftware:main
Choose a base branch
from
mcfnord:client-connection-state
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 ), | ||
|
|
@@ -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 ); | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -905,13 +909,10 @@ void CClient::OnHandledSignal ( int sigNum ) | |
| { | ||
| case SIGINT: | ||
| case SIGTERM: | ||
| // if connected, terminate connection (needed for headless mode) | ||
| if ( IsRunning() ) | ||
| { | ||
| Stop(); | ||
| } | ||
| // tear down any pending or established connection first, so the server | ||
| // is notified we are leaving (Disconnect() is a no-op if not connected) | ||
| Disconnect(); | ||
|
|
||
| // this should trigger OnAboutToQuit | ||
| QCoreApplication::instance()->exit(); | ||
| break; | ||
|
|
||
|
|
@@ -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 ); | ||
| } | ||
|
|
||
|
|
@@ -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 | ||
|
|
@@ -1088,6 +1103,74 @@ 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 Disconnects from the server if a connection is requested or established. | ||
| /// Idempotent: a no-op when already disconnected. | ||
| /// @emit Disconnected | ||
| void CClient::Disconnect() | ||
| { | ||
| // 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. | ||
| if ( GetConnectionState() != CS_DISCONNECTED ) | ||
| { | ||
| 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() ); | ||
| } | ||
| } | ||
|
|
||
| /// @method | ||
| /// @brief Updates the connection state and, if it changed, notifies observers. | ||
| /// @emit ConnectionStateChanged (state) when the state actually changes | ||
| /// @param eNewConnectionState - the state to transition to | ||
| void CClient::SetConnectionState ( const EConnectionState eNewConnectionState ) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add documentation like for connect/disconnect.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done in efee722 — added a |
||
| { | ||
| if ( eConnectionState != eNewConnectionState ) | ||
| { | ||
| eConnectionState = eNewConnectionState; | ||
| emit ConnectionStateChanged ( eConnectionState ); | ||
| } | ||
| } | ||
|
|
||
| void CClient::Init() | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 —
OnHandledSignalnow callsDisconnect(), and the state-based guard means SIGTERM whileCS_CONNECTINGstill sendsCLM_DISCONNECTION(verified by capture).There was a problem hiding this comment.
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()isSound.IsRunning()(the audio device).Start()setsCS_CONNECTINGand callsSound.Start()as separate steps, so the two diverge: duringCS_CONNECTING, and in headless mode where audio may lag or be unavailable, the state is connecting/connected whileIsRunning()is still false.Stop()is the only thing that sendsCLDisconnection, and on the client there is noaboutToQuithandler and~CClient()only doesSound.Stop()— soOnHandledSignalwas calling rawStop()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():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 OnAboutToQuitcomments (there is no clientOnAboutToQuit).Verified with a UDP capture: SIGTERM while connected → server receives
CLM_DISCONNECTION; SIGTERM while idle → no spurious disconnect (the old rawStop()sent one to a stale address); cleanexit 0in both cases.