[pulseaudio] Apply real disconnection when needed

[dalgwen]

Bug description:
When the pulseaudio server is shutdown and relaunched, the socket is dead. But the audiosink connection doesn't properly reset the next time it is needed. The isIdle boolean prevents this disconnection/reconnection by checking if it's in use before effectively disconnecting (and of course it's in use, because we are trying to use it when we see that the connection is broken...)
Additionnally, isIdle is not in a try catch block for the pulseaudio sink : so exception within an operation doesn't free the boolean.

Actually, isIdle seems not even relevant : we should always honnor the disconnection request, even if an operation is running (disconnection requests occur when we get I/O exception, or when shutting down the bridge/thing).

This PR remove the isIdle boolean.

+Little bug fix on volume parsing: in some case (audio source), a volume request doesn't respond with a space after the comma separating left/right channel, so I change the volume pattern.

[GiviMAD]

The "isidle" logic attempts to prevent closing the connection for a while to speed up the response time when you are using it frequently (for example when using it with the dialog processor implementation), it was not implemented by me but doesn't seems like something we want to remove.
I think it will affect the performance of the binding for people which is already using it, as you will be tearing up and down the pulseaudio "Simple TCP Stream" protocol port on each use.

[GiviMAD]

The "isidle" logic attempts to prevent closing the connection for a while to speed up the response time when you are using it frequently (for example when using it with the dialog processor implementation), it was not implemented by me but doesn't seems like something we want to remove.
I think it will affect the performance of the binding for people which is already using it, as you will be tearing up and down the pulseaudio "Simple TCP Stream" protocol port on each use.

Have reviewed the code and I was wrong, the disconnect method just closes the socket, do not instruct the pulseaudio server to close the "Simple TCP Stream" protocol port, but still the next usage will require to check if this port is available plus reopening the connection so I don't think is a bad idea to keep the "isidle" logic if we can fix the issue in another way.

[dalgwen]

Thank you for checking this PR !
You point me to something I forgot (case 3 below)

So, if I want to sum up, as far as I understand it, the isIdle boolean is :

• set to false when we want to use the socket
• set to true when it is finished
• Its only purpose is to be checked against, when something tries to disconnect (false --> do not close, true --> ok to close)

So it prevents the socket from being disconnect, when we are playing or recording a sound.

The situations, when we want to disconnect, are :
1- getting an IO exception --> we should force close it now, even if isIdle is false. Currently the boolean prevents the closure and the socket is in a broken state until the scheduled time out closure (which in some case could never happen, if the timeout is set to -1)
2- disabling the sink / source / bridge / bundle / whatever --> we should force close it, even if isIdle is false. Currently it is not closed: if we try to disable the thing when a sound is playing, the socket is kept open (possibly forever if the timeout is set to -1)
3- the idle timeout is expired --> Indeed, if isIdle in false, we should NOT close it. In fact we should NOT even try to. The idle timeout and the associated scheduledDisconnection should be canceled/postponed the exact moment we start to use the socket.

I think we should implement it this way: canceling the scheduledDisconnection when the socket is opened/used, instead of not closing it at the scheduledDisconnection timeout expiration if the socket is in use.
If we do it like this, we don't need the isIdle boolean, and its side effect bug with the other close cases are gone.

I will add a commit to this PR to take the third point in consideration.

[lolodomo]

I will add a commit to this PR to take the third point in consideration.

So, PR is not yet ready for review & merge ?

[dalgwen]

This PR is now ready for a second review.

After the judicious comment from @GiviMAD , I complete my initial proposal with a rewrited system for the idle -> disconnection functionnality.

Instead of simply deleting the isIdle variable (which as I said in the OP was faulty and prevented in some case a needed healing disconnection) I now propose using a count of the clients using the connection, and giving it the responsibility of scheduling optional disconnection.
The isIdle variable was not thread safe, and a thread safe count seams a better logic system. It works like this :

• When the count is down to 0 (i.e. when the last client finish its operation), a future disconnection is scheduled.
• When the count is up (a client initiate an operation), then the future disconnection is canceled.
  AFAIK, it ensures that :
• no disconnection can happen if the source/sink is still in use
• a disconnection is always scheduled after all operations are done.

Alternatively, the connectIfNeeded method can also schedule a disconnection. Indeed, using solely the end of an operation to schedule a disconnection is not sufficient because the handler also initiate a dumb connection at startup, without operation, with the goal to check connectivity.

I tested this solution with a sink (with a TTS) and with a source (using rustpotter KS and volk STT) and it works.

Additionnaly, I made a little enhancement to avoid using a synchronized method for a method called very often.

[GiviMAD]

Hey @dalgwen, I let you a couple of comments, but it seems like a great improvement, thank you!

[GiviMAD]

LGTM!
Thanks for the changes. I'm using this binding on a daily basis to connect to a speaker at my living room for using the voice assistant capabilities and I think I have faced the issue couple of times, so really appreciated!

[lolodomo]

Code LGTM, I have not tried to understand the detailed stuff with "idle" but as @GiviMAD already approved the changes, that is fine for me.