jamulussoftware
Refactor Connect and Disconnect functionality out to CClient
This is an extract from https://github.com/jamulussoftware/jamulus/pull/2550
Short description of changes
Refactor Connect() and disconnect functionality. Introduces a new Connect() method for quick connection and adds signals to Start() and Stop() of client.
CHANGELOG: Refactoring: Move Connect() functionality to CClient.
Context: Fixes an issue?
Fixes: #3367
Does this change need documentation? What needs to be documented and how?
No
Status of this Pull Request
Ready for review. Start for refactoring. Most likely overlaps and to a large extent relates somehow to https://github.com/jamulussoftware/jamulus/pull/3364
What is missing until this pull request can be merged?
Review, clarification, probably documentation and refactoring.
Checklist
- [x] I've verified that this Pull Request follows the general code principles
- [x] I tested my code and it does what I want
- [x] My code follows the style guide
- [x] I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
- [x] I've filled all the content above
Now the coding style check seems to work. Not so sure about the style change in main.cpp: https://github.com/jamulussoftware/jamulus/actions/runs/10982471650/job/30490757878?pr=3372#step:4:54
Please recheck if the important parts are in the two issues I opened and comment there.
I do not understand why clang format complains about the list style here.
https://github.com/jamulussoftware/jamulus/actions/runs/11242233032/job/31255455777?pr=3372#step:4:18
OK... so what I'm expecting to see...
- sort out "exit client" so that there's only "one way out"
- there should be one CClient method that tells Qt the client is exiting and does nothing else.
- this would be called from GUI (File->Exit) and SIGINT/SIGTERM handlers to make the client close and those would do nothing else.
- any and all other cleaning up should happen when Qt sends the "OnAboutToQuit" signal.
- there should be one CClient method that tells Qt the client is exiting and does nothing else.
- work out what "disconnecting from a server" means
- have a CClient method that performs disconnection
- does nothing if not connected
- tells the server the client is disconnecting (?? do we do this ??)
- calls another CClient method to handle disconnecting
- have CClient respond to the server disconnecting by calling another CClient method to handle disconnecting (lambda)
- have a CClient method that handles disconnecting by
- cleaning up any CClient structures
- emitting a (blocking) Disconnected signal
- amend other components to listen for Disconnected and take appropriate clean up actions
- have a CClient method that performs disconnection
- work out what "connecting to a server" means
- have a CClient method that
- calls the CClient method to perform disconnection
- initiates the connection (which could fail)
- emits a signal indicating a connection has been requested but not yet granted
- have a CClient method that handles a successful connection
- have a CClient method that handles an unsuccessful connection
- have a CClient method that
I think most of the Connect stuff is there. I'm less clear what's happening around Disconnect. And the exiting the client currently appears very messy - at least, it's confusing me...
Yes, I agree. This may imply that we may need to restructure the UI functionality a bit. I'll have a look in a few weeks. Currently I have a higher load with another development project.
I would like to define the methods as follows (not sure if this is everything)
CClient is connected iff all of those hold
- CClient.Init() was called
- CClient.Connect() was called
- Channel.IsEnabled() is true
- Sound.IsRunning() is true
- CClient.IsRunning() is true (which is to be redefined)
- There is no soundcard error (to be defined)
CClient will disconnect if at least one of these conditions holds
- A soundcard error occurs
- The server disconnects
- Disconnect() was called
CClient is running iff
- CClient.Init() was called
- CClient.Start() was called
- CClient.IsRunning() returns true
- Potentially: SetServerAddress() was called and succeeded
This means, there may be a sound card error while CClient is running and the client may not be connected to a server
To transition from CClient being running to CClient being connected, we need to call Connect() and the connection to the server must stand (within the timeout). To transition from CClient being connected to just being running, we have to:
- Either call Disconnect()
- Have any kind of network or audio issue
- The Server requests a disconnect
A SIGINT would
- Stop the client
- Which will force the client to disconnect.
To transition from CClient being connected to cclient being disconnected, We'd just call disconnect. I do not think there's a way to have a failed disconnection:
This means:
- Sound.Stop() gets called and Channel.SetEnable(false) gets called, ConnLessProtocol.CreateCLDisconnection() gets called. Only after ConnLessProtocol.CreateCLDisconnection() the client is disconnected (and should not accept any packets from the server anymore). It is only in stopped state after the GUI is finished (SignalLevelMeter.Reset() was called - I don't like to have this in CCLient though.)
Does this make sense?
Background
(Apologies for writing this out in a PR where you know it as well as I do but I'm trying to make sure my thoughts are in order...)
We need states:
- disconnected
- connection requested but not connected
- connection established and current
"Disconnected" means no connection has been requested or established "Connection requested but not connected" means a connection has been requested but audio data is not being exchanged yet "Connection established and current" means a server connection is in progress
Now, these states should apply equally to the Client's view of it's single connection to a Server and Server's view of each of its connections to a Client. This is important (and a core concept to Jamulus).
The Client Channel owns that state. The Client has one Channel but the Server has many Channels. So any changes here must not break the Server.
A Channel is considered disconnected when no more audio data has been exchanged for a certain period (or something, I don't remember this bit) or an explicit disconnect is received (maybe, I'm not sure on that, either). Again, that goes equally for both ends of the Channel.
(And finally... The Channel carries Protocol messages that can either be audio or other messages. The audio messages go into the jitter buffer, triggering audio processing. The other protocol messages get processed, quite often by the Channel itself but sometimes needing either Client or Server (as they know more about the world) to get involved. The main difference between Client and Server, of course, is audio processing -- the Client exchanges CSound audio data with the jitter buffer but the Server pushes the audio through the mixer.)
CClient is connected iff all of those hold
OK, let's start with "CClient has a CChannel" and it's "CChannel" that holds state regarding whether an active connection has been established. Given this is the case (and it has to be for the Client and Server to work using the same code):
CClient is "connected" or "running" or "started" or whatever, if and only if
- its CChannel.ConnectionState() is Connected (or "CChannel.IsConnected()")
- its CSound.IsRunning() is true
Neither of those alone is useful for components outside CClient (other than CChannel and CSound maintaining their own state). Nothing else needs worrying about.
CClient.Connect() was called
Maybe we rename CClient.Connect(...) to CClient.RequestConnection(QString server) because that's where all the Client-specific logic lives and it doesn't end up by connecting. It would:
- always call a CClient.EndConnection() method that
- calls CChannel.Disconnect()
- (somewhere drains the out-going and in-coming audio packets in the jitter buffer - I guess it's here)
- calls CSound.Stop()
- (by this point it should have had one packet of silence to output)
- does any other Client-specific clean up (e.g. clear connected server name)
- emit a ConnectionEnded signal
- calls CChannel.Disconnect()
- calls CSound.Start()
- calls CChannel.RequestConnection(QString server)
- emits a ConnectionRequested(QString server) signal
CChannel.Connect() is Client-only. It would
- set connection state of ConnectionRequested (so CChannel.ConnectionState() returns that -- IsConnected() would be false)
- set up the connection to the server
- fills the jitter buffers with silence and keeps them that way until the connection is established
- open the socket and set up the signal handlers
- kill the connection if a timer expires before the timer is stopped
(The Client is responsible for putting audio from CSound into the channel jitter buffer, which should trigger CChannel to take audio from the jitter buffer and send it to the Server, and for reading the jitter buffer and passing the audio to CSound. I don't clearly remember the structure here. There's a timer that expires if processing doesn't start. There's a timer that expires for each audio cycle, too. I think both are in CChannel as it's very similar in Client and Server.)
CChannel will handle audio being received by marking the CChannel connection state as ConnectionEstablished (IsConnected() is true). Again, same at Client and Server for this. It also emits a ConnectionEstablished signal.
CClient will disconnect if at least one of these conditions holds
The first two should cause CClient.Disconnect() to be called and nothing else - lambda slot handlers in their respective classes calling the client method for signals. It'll get called during CClient.OnAboutToQuit, I expect.
Actually, I'm proposing CClient.EndConnection() in place of CClient.Disconnect() to make it clear it's a non-trivial operation. It should also always, without condition:
- call CChannel.Disconnect() (which may itself check whether it's in a ConnectionEstablished state)
- (somewhere drains the out-going and in-coming audio packets in the jitter buffer - I guess it's here)
- call CSound.Stop() (which may itself check whether it's running)
- (by this point it should have had one packet of silence to output)
- do any other Client-specific clean up (e.g. clear connected server name)
- emit a ConnectionEnded signal
CClient is running iff
We need two things:
- CChannel.ConnectionState() should be ConnectionEstablished
- CSound.IsRunning() should be true
We could also check the wiring up of the signals from those two classes to the slots in the Client are in place so the sound goes to/from the channel... because CChannel and CSound do not know CClient is handling their signals. However, that would be wired up once during creation of CClient, so can be taken as a given.
Nothing else matters because, so long as we have sound going in and out and packets going in and out - that means we set up okay and we're running fine.
This means, there may be a sound card error while CClient is running and the client may not be connected to a server
Yes - but by CChannel changing state from ConnectionEstablished to Disconnected and emitting a ConnectionEnded signal or by CSound changing state from Running to Stopped and emitting an SoundStopped signal, where both are handled by CClient. In either case, asking CClient whether it's "connected" or "running" or "started" or whatever, it'll say false.
In fact, CChannel::ConnectionEnded and CSound::SoundStopped would get handled the same way -- calling CClient::EndConnection. (Or StopConnection or something... it needs a better name...)
To transition from CClient being connected to just being running, we have to:
OK, I don't understand your concept of "just being running". The Client just being there isn't the Client being in any particular state in and of itself. It's just there, handling signals. That doesn't need a state in and of itself.
- If CChannel.ConnectionState() should - unexpectedly - transition from ConnectionEstablished to anything else, we terminate the connection and stop sound -- because it means we lost the connection (timer expired).
- If CSound.IsRunning() should - unexpectedly - transition from true to anything else, we terminate the connection and stop sound -- because we got an audio error.
Neither of the above changes the state of the Client itself -- it would change the result of the method that combines the result of CChannel.ConnectionState() and CSound.IsRunning(), though. But that's not a state, it's a computation.
If CChannel gets a server disconnect protocol message, it'll need to pass that to CClient to deal with - not simply disconnect, as that would be seen as an error happening.
Any request to Disconnect would be handled the same way -- call CClient.EndConnection().
Exiting the client can happen multiple ways -- CLI through SIGINT/SIGTERM, GUI through menu emitting a signal, or a JSON-RPC command. These should all trigger the Qt OnAboutToQuit by asking Qt to exit the application. The Window Manager can also trigger Qt OnAboutToQuit by asking Qt to exit the application. In all cases, when the client terminates, part of handling OnAboutToQuit in CClient would be to call CClient.EndConnection().
Or maybe I'm missing something?