lcu-driver
lcu-driver copied to clipboard
sousa-andre
Fix bugs regarding connector.start() & connector.stop()
-
connector.stop() bug: The change prevents the occurrence of an infinite loop, which would previously happen if the @connector.ws.register() decorator was used to register a connection. This is done by storing each WebSocket registered by the user in a list called self.connections. Which when wanting to
.stop()allows us to first iterate over each stored connection object and unregistering stale websocket connections, so that the stop method can successfully close the connections as intended. (fixes https://github.com/sousa-andre/lcu-driver/issues/18) -
connector.start() bug: When the League Lobby Clients setting "Close client during game" was set to "Always"; it resulted in the in-game client being open exclusively during games. This caused the initialization of lcu-driver's connector.start() method to fail because the LeagueClientUx process was not running. This change introduces automatically killing the in-game client, if it was detected as running during initialization of lcu-driver, this action triggers the automatic relaunch of the LeagueClientUx process, finally enabling lcu-driver to start as intended.
I just realised that now with my changes in https://github.com/sousa-andre/lcu-driver/pull/34/commits/ee3f4d340a75b578446d7eeb5aa5004b5bbb4031, if await connector.stop() is manually called somewhere, instead of within a function with the @connector.close decorator, there will be a value error.
So a hacky fix would be:
def unregister_connection(self, _):
try:
self.connections.remove(self)
except ValueError:
pass
self.connection = None
Edit: Added that here, you might want to solve this issue more smoothly though :)
I just realised that you've a completly undocumented & unmentioned class in connector.py called MultipleClientConnector which perfectly implements everything I was trying to implement into the Connector class.
Just write something similar to how you've kept track & cancelled previously registered websocket connections in MultipleClientConnector, into the Connector class and that will fix #18
I would still also be happy about a merge of the change I labeled as connector.start() bug above though, as that change is actually really usefull.
Edit: I noticed the MultipleClientConnector class didn't have a stop event similar to the Connector class, so I added one and now it can actually terminate and re-start as intended in https://github.com/sousa-andre/lcu-driver/pull/34/commits/4b81934a08101f103ee8aba32585ef11188d4772 Previously it wouldn't actually close out and instead would just get stuck doing nothing whenever await conn.stop() was called. Also I've reverted all changes I did to the Connector class and have just given up on using it, the stale websocket thing in it is just going to persist. Atleast MultipleClientConnector works as intended now and I'm personally switching over to using it instead.
Firstly, thank you for taking the time to open the pull request; it's great to have someone to help 😄.
Regarding the .stop() issue and the undocumented class MultipleClientConnector, I added that class to the library to solve a specific problem that some people were facing (I don't remember exactly which). Since it's close to impossible to cover all use cases of the library, I think it's better to write documentation on how to extend the BaseConnector class to fit your needs and not to create classes for specific use cases.
As for the start bug, I agree that it needs to be fixed, but I don't think closing the game itself is the way to go, at least not by default. I would love to hear your opinion on some solutions for this.
The pull request seems alright even though some commits should have been addressed in a different pull request.
Regarding the .stop() issue and the undocumented class MultipleClientConnector, I added that class to the library to solve a specific problem that some people were facing (I don't remember exactly which).
Could this be the exact issue I was referring to with the websockets? Because the only difference between your connector class and the MultipleClientConnector (MCC) class is that MCC handles tracking and closing of websockets effectively. With my additions to the MCC class, it allows for stopping and restarting lcu-driver as intended. Merging that change would be beneficial.
As for the normal Connector class, the issues I mentioned with websockets not closing properly persist. This prevents lcu-driver from closing at all if a websocket has been registered during lcu-driver's runtime. You may consider modifying the Connector class yourself, but since MultipleClientConnector already works well, renaming it to Connector and removing the other buggy class seems like a viable option. I mean both classes are quite similar, except one works and the other doesn't.
As for the start bug, I agree that it needs to be fixed, but I don't think closing the game itself is the way to go, at least not by default. I would love to hear your opinion on some solutions for this.
Yes this is the same conclusion I've came to after a week of testing the change (which is why I reverted the changes to utils.py). The only idea I personally could come up with, is exactly defining the time when each client may be open and when not, but I'm not sure if such a functionality should be directly apart of lcu-driver as it's kind of a niche use case.
As for the normal Connector class, the issues I mentioned with websockets not closing properly persist. This prevents lcu-driver from closing at all if a websocket has been registered during lcu-driver's runtime. You may consider modifying the Connector class yourself, but since MultipleClientConnector already works well, renaming it to Connector and removing the other buggy class seems like a viable option. I mean both classes are quite similar, except one works and the other doesn't.
The Connector class should only handle a single connection for the entirety of a client connection and calling close on it only stops it from looking for new league client instances when one is closed.. Can you give me more information on the issue you are facing?
Can you give me more information on the issue you are facing?
Register a websocket and use the normal Connector class, then try to close lcu-driver and see if it closes or just gets stuck doing nothing, because the stale websocket connection is dissalowing it to close.
calling close on it only stops it from looking for new league client instances when one is closed.
Oh are you saying this is intended this behaviour; where once lcu-driver has started, it can never be entirely stopped again? Only the seraching is stopped, but you may never return to normal global python interpretation and stay stuck with lcu-driver being stale and doing nothing?
Register a websocket and use the normal Connector class, then try to close lcu-driver and see if it closes or just gets stuck doing nothing, because the stale websocket connection is dissalowing it to close.
When you register a connection with a websocket it should block until the client closes or the user calls the .stop method.
Oh are you saying this is intended this behaviour; where once lcu-driver has started, it can never be entirely stopped again? Only the seraching is stopped, but you may never return to normal global python interpretation and stay stuck with lcu-driver being stale and doing nothing?
When you call the .stop method on a Connector class instance, not only does the connector stop looking for new clients/connections, but it also interrupts the current connection that's listening to incoming WebSocket messages from blocking. As a result, the execution returns back to the global Python interpretation. More on that here
When you call the .stop method on a Connector class instance, not only does the connector stop looking for new clients/connections, but it also interrupts the current connection that's listening to incoming WebSocket messages from blocking. As a result, the execution returns back to the global Python interpretation. More on that here
Except that this doesn't work, unless registered websockets are actively tracked and closed when a close event is received, else they end up stale, blocking lcu-driver from closing. Which is the whole point I've added these commits; MCC already had the tracking neccessary, but it was lacking the stop event to actually close them all out.
Look MCC can already track multiple registered websockets within a list. https://github.com/sousa-andre/lcu-driver/blob/768d79b81ba7fbfbd590b3f226d37c5a86a7f0e1/lcu_driver/connector.py#L88
While the normal connector class only tracks the latest registered websocket (?) https://github.com/sousa-andre/lcu-driver/blob/768d79b81ba7fbfbd590b3f226d37c5a86a7f0e1/lcu_driver/connector.py#L38
When you call the .stop method on a Connector class instance, not only does the connector stop looking for new clients/connections, but it also interrupts the current connection that's listening to incoming WebSocket messages from blocking. As a result, the execution returns back to the global Python interpretation. More on that here
Except that this doesn't work, unless registered websockets are actively tracked and closed when a close event is received, else they end up stale, blocking lcu-driver from closing. Which is the whole point I've added these commits; MCC already had the tracking neccessary, but it was lacking the stop event to actually close them all out.
Look MCC can already track multiple registered websockets within a list.
https://github.com/sousa-andre/lcu-driver/blob/768d79b81ba7fbfbd590b3f226d37c5a86a7f0e1/lcu_driver/connector.py#L88
While the normal connector class only tracks the latest registered websocket (?)
https://github.com/sousa-andre/lcu-driver/blob/768d79b81ba7fbfbd590b3f226d37c5a86a7f0e1/lcu_driver/connector.py#L38
You're right about the issue with the .stop method. In the context of the connector, the .stop method only prevents the connector from finding new clients and doesn't stop any connection. I'm considering creating a new method on the Connection class to stop the run_ws method (a simple flag should solve this).
The Connector class handles only one connection at a time, so there's no point in maintaining a list to track every client/connection that've been open. The connector shouldn't interfere with the lifecycle of the connections. Dealing with multiple clients inside this class potentially leads to more issues, such as keeping track of new clients that might attempt to open on the same port as a previous one.
Sorry for the delayed feedback. I gave this a second though and I would be down to abstract the connector classes as much as possible and create some implementations to handle the most generic cases such as the one you have.