vinenoobjelly / jellyfishlights-py

12 stars 5 forks source link

Async data retrieval #5

Closed bdunn44 closed 1 year ago

bdunn44 commented 1 year ago

Following up on PR #4, this is a new way to listen for data updates coming over the websocket that eliminates any concern about multiple responses per request. I've also observed that the controller will broadcast certain responses to all active websocket connections (not just to the requesting connection), so this enables multiple active connections to work independently without any concern of a response to a request from one connection impacting another connection. No timeouts or race conditions with this approach.

I've tested this with two threads, each fetching data and performing actions without any issue. Happy to iterate on these changes as needed.

bdunn44 commented 1 year ago

Looked at this code again... It's probably not a big deal, but the data attributes could use a lock (or multiple) to ensure thread safety when reading & writing response data. I'll look into adding this

bdunn44 commented 1 year ago

@vinenoob, thoughts on this? It probably seems overly complex for a straightforward problem, but as mentioned in #4 this is actually what is recommended by websocket-client's author in https://github.com/websocket-client/websocket-client/issues/416 and in the documentation.

vinenoob commented 1 year ago

@bdunn44 I've looked at your solution and am considering it. What if instead we had a receiving thread that set the variables in the object? The pattern would be something like this: when we want data, we send the request and then wait for the receiving thread to somehow mark that it received the relevant data. I haven't tried it out and I need to think about it some more but I think something like that could be the solution. My wording my not come across clearly so I'm hoping to get to trying it out (but feel free to do it yourself if you understand what I'm aiming for). Edit: This may be very close to #4

vinenoob commented 1 year ago

Also while I have looked at the proposed solution, I haven't studied it super in depth as of yet

bdunn44 commented 1 year ago

@vinenoob that's actually exactly what this PR does! Basically:

  1. The connect() function: a. Creates the WebSocketApp object with callbacks that are called when data is received or the connection state changes (__ws_on_open, __ws_on_close, __ws_on_message). b. Starts a new thread that executes WebSocketApp#run_forever(), which essentially establishes the web socket connection and listens for messages indefinitely.
  2. We have a set of Event objects (stored in the __connected and __events variables) that signal the main thread when certain events happen (when the connection state changes, and when the various messages we care about are received).
  3. __ws_on_message does the heavy lifting. This function is called when a message is received by the background thread. It essentially: a. Detects what type of data is contained in the message b. Parses the data c. Stores the data in the object (the zones, patternFiles, and runPattern variables) d. Notifies the main thread that data has been received by triggering the Event objects
  4. When new data is requested (by getRunPattern(), for example): a. The request message is sent on the web socket b. It waits for the Event to be triggered c. Then returns the data stored on the object

There is also a set of Locks that ensure data is not being read and written by the two threads simultaneously.

Hope this helps you dissect the code a bit. We're definitely on the same page about how to solve for this, though.

vinenoob commented 1 year ago

I'm just gonna go forward with this but we may investigate other options in the future. Honestly my biggest problem with it is I don't 100% understand it (details, I get the general idea), but that probably shouldn't stop us

bdunn44 commented 1 year ago

Thanks for the merge @vinenoob. Where's it hazy for you? The background thread? The callbacks? Events? Locks? I'd be happy to answer any questions - it's your repo after all 🙂