create new DeviceListReceived event to avoid race conditions in short–lived client applications

[slonkazoid]

The Problem

Currently, the response handling for the DeviceListRequest call emits an internal event which is processed asynchronously, and can not be awaited for in the crate's public API. This forces clients to wait for DeviceAdded events to know when to actually start using the client. The internal implementation looks like this:

buttplug/buttplug/src/client/client_event_loop.rs

Lines 308 to 318 in d3247fb

	ButtplugClientRequest::HandleDeviceList(device_list) => {
	trace!("Device list received, updating map.");
	for d in device_list.devices() {
	if self.device_map.contains_key(&d.device_index()) {
	continue;
	}
	let device = self.create_client_device(d);
	self.send_client_event(ButtplugClientEvent::DeviceAdded(device));
	}
	true
	}

This is not ideal, as a client application will not be notified about when the device list is received if there are no devices connected. A workaround for this is just to sleep() for a while to make sure the event is handled. This is fragile and slow for reasons that I do not think require explaining.

The Solution

The solution I propose in this PR is to add a DeviceListReceived event that client applications may listen to
in order to ascertain that devices have been enumerated:

diff --git a/buttplug/src/client/mod.rs b/buttplug/src/client/mod.rs
index 5af9ebaa..f0bebf96 100644
--- a/buttplug/src/client/mod.rs
+++ b/buttplug/src/client/mod.rs
@@ -128,6 +128,9 @@ pub enum ButtplugClientEvent {
   /// Emitted when a device has been removed from the server. Includes a
   /// [ButtplugClientDevice] object representing the device.
   DeviceRemoved(Arc<ButtplugClientDevice>),
+  /// Emitted when the device list is received as a response to a
+  /// DeviceListRequest call, which is sent during the handshake.
+  DeviceListReceived,
   /// Emitted when a client has not pinged the server in a sufficient amount of
   /// time.
   PingTimeout,

This event is emitted by the previously mentioned internal event loop, after all the DeviceAdded events have been dispatched:

diff --git a/buttplug/src/client/client_event_loop.rs b/buttplug/src/client/client_event_loop.rs
index 6b6bc6d5..caf8e53b 100644
--- a/buttplug/src/client/client_event_loop.rs
+++ b/buttplug/src/client/client_event_loop.rs
@@ -314,6 +314,7 @@ where
           let device = self.create_client_device(d);
           self.send_client_event(ButtplugClientEvent::DeviceAdded(device));
         }
+        self.send_client_event(ButtplugClientEvent::DeviceListReceived);
         true
       }
     }

This PR would require a minor version bump as the events enum is not marked #[non_exhaustive]. Consider doing that too.

Extras

• a .device_list_received() fn on the ButtplugClient object that returns whether a device list has been received or not

[CLAassistant]

All committers have signed the CLA.

[qdot]

Just trying to make sure I understand this PR, is this to let the user know that all DeviceAdded/DeviceRemoved events have happened at the beginning of a connection session? 'cause there's no way to call RequestDeviceList otherwise. The assumption is currently that, after the connect call exits, you can assume all connected devices are in the client's devices list, and the DeviceAdded calls there were just a formality.

[slonkazoid]

The assumption is currently that, after the connect call exits, you can assume all connected devices are in the client's devices list

I assumed so as well, but, this is not the case. There's a race condition between when the internal event loop processes the HandleDeviceList event and the call exits. Another way of fixing this would be to pass a oneshot tx into the HandleDeviceList event and waiting on the rx end.

[slonkazoid]

As I have stated, the current approach works for long–running applications that connect once and can spare the time to wait for events, but not clients that connect, set state, and exit without waiting.

[qdot]

As I have stated, the current approach works for long–running applications that connect once and can spare the time to wait for events, but not clients that connect, set state, and exit without waiting.

Not sure I understand how this works? If you disconnect a client after setting state, all devices should stop? What's the use case here?

[slonkazoid]

Not sure I understand how this works? If you disconnect a client after setting state, all devices should stop? What's the use case here?

The devices list is empty when you connect because

buttplug/buttplug/src/client/mod.rs

Line 360 in d3247fb

.send_message_to_event_loop(ButtplugClientRequest::HandleDeviceList(m))

is handled asynchronously. I want to connect, get a complete device list ASAP, activate one or more devices, wait a specified amount time, stop, exit.

2 approaches to fix this:

• A: new client event
• B: wait for it to get handled before returning from run_handshake

this PR implements approach A, but approach B would also be easy to implement, and wouldn't require a minor version bump

Edit: upon a second review it seems that approach B is much harder to implement actually

[qdot]

Got it, thanks. I'm digging into this is because the library has a bunch of huge changes coming up (see the dev branch) and I wanted to make sure of the need here and how much of our usage it covers, especially because we drop the wire protocol versions of Added/Removed and now only send DeviceList.

Granted almost no one uses the rust client anyways so I also rarely get feedback on that heh.

Anyways, I'll see if there's some way to at least ensure we have devices when connect() returns, though I'll have to make sure of what the best way to do that is in relation to the new work that's come in on dev.

[slonkazoid]

tysm for all the work!

[slonkazoid]

i've drawn a diagram illustrating the issue