Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not automatically start worker thread and connect in Chromecast constructor #271

Merged
merged 7 commits into from Mar 9, 2019

Conversation

emontnemery
Copy link
Collaborator

@emontnemery emontnemery commented Mar 1, 2019

Do not automatically start worker thread or connect in Chromecast constructor.

This is a breaking change, and the user will now have to call either of:

  • Chromecast.start(): Start the worker thread and connect to the chromecast device. Connection status will be reported through the listener registered in Chromecast.register_connection_listener.
  • Chromecast.wait(): Wait for connection, this will also start the worker thread if it has not been started.
  • Chromecast.connect(): Connect to the chromecast. This must only be called if the worker thread has not been started. Connection status will be reported through the listener registered in Chromecast.register_connection_listener.

Background:
The automatic connect in the constructor meant that the constructor would hang forever if the number of retries was unlimited and the chromecast could not be found.
It was also a bit unnatural to start the worker thread in the constructor.

@balloob
Copy link
Collaborator

balloob commented Mar 2, 2019

When do we want to connect right away? Could we just do the connect call only inside the discover methods and not do any I/O in the constructor? (which shouldn't do I/O really)

@emontnemery
Copy link
Collaborator Author

emontnemery commented Mar 2, 2019

@balloob sure, but I guess this library is used by many people. Do you think it's ok to change the behaviour?

Edit: With current behavior, the constructor throws ChromecastConnectionError if it can't connect to the cast (when tries is limited). I do agree it makes more sense to only do the blocking I/O from SocketClient's thread, which is anyway also started from the constructor.
It will break applications relying on the Chromecast constructor throwing though.

@balloob
Copy link
Collaborator

balloob commented Mar 3, 2019

Ah right, now I remember the usage. It is so that when you instantiate, you know if it works or not. I suggest we make this a breaking change and a major bump. Require people to manually call initialize_connection

@emontnemery
Copy link
Collaborator Author

Require people to manually call initialize_connection

initialize_connection is automatically called if we're disconnected from the cast, do you suggest maintaining the connection is now the user's responsibility? Or just the initial connect? If the latter, I think the call should rather be to start the worker thread.

@balloob
Copy link
Collaborator

balloob commented Mar 4, 2019

ah good one. Maybe we can do that?

@emontnemery
Copy link
Collaborator Author

Maybe we can do that?

I'm not sure what that means in this case, do you just suggest ro remove this:
https://github.com/balloob/pychromecast/blob/ec0775a5aab56378f0761be01661fbb5fa74119e/pychromecast/__init__.py#L214-L215
And the user has to make that call instead? I'm not sure if it's very useful.

What may be useful though is to make the change in this PR, i.e. defer_connect=True the default and only supported mode?

@balloob
Copy link
Collaborator

balloob commented Mar 4, 2019

Yes, let's make it the only supported mode. We will do a major version bump. We should make sure we update our examples to show how existing code can be updated

README.rst Outdated
@@ -39,6 +39,8 @@ How to use
['Dev', 'Living Room', 'Den', 'Bedroom']

>> cast = next(cc for cc in chromecasts if cc.device.friendly_name == "Living Room")
>> # Start worker thread
>> cast.start()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking here, do you think that we can automatically call start inside wait, if not started yet ? That way it's a less breaking change.

@balloob
Copy link
Collaborator

balloob commented Mar 8, 2019

Could you write a message describing the breaking change for the release post?

@emontnemery
Copy link
Collaborator Author

Sure, where do I write it?

@balloob
Copy link
Collaborator

balloob commented Mar 8, 2019

As a comment here :D

@emontnemery emontnemery changed the title Add defer_connect option to Chromecast class constructor Do not automatically start worker thread and connect in Chromecast constructor Mar 9, 2019
@emontnemery
Copy link
Collaborator Author

emontnemery commented Mar 9, 2019

OK, I updated the issue description to be clearer, I think it can be used for the release post.

@balloob balloob merged commit b1cd262 into home-assistant-libs:master Mar 9, 2019
@balloob
Copy link
Collaborator

balloob commented Mar 9, 2019

Released as 3.0.0

@strunker
Copy link
Contributor

strunker commented Mar 12, 2019

Struggling a bit getting 3.0 working again, for now going to revert to 2.4 or 2.5

Running through the simple example on the landing page I get the below:

import pychromecast
chromecasts = pychromecast.get_chromecasts()
cast = next(cc for cc in chromecasts if cc.device.friendly_name == "Kitchen")
cast.connect()
mc = cast.media_controller
mc.play_media('http://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4', 'video/mp4')

Calling cast.wait() or cast.connect()
Results in the below error.

Traceback (most recent call last):
File "c:\python27\lib\threading.py", line 801, in __bootstrap_inner
self.run()
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 423, in run
self.initialize_connection()
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 252, in initialize_connection
for service in self.services.copy():
AttributeError: 'list' object has no attribute 'copy'

Calling cast.start() results in the below error:

Traceback (most recent call last):
File "C:\Python27\Echo\test.py", line 466, in
mc.play_media('http://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4', 'video/mp4')
File "c:\python27\lib\site-packages\pychromecast\controllers\media.py", line 480, in play_media
callback_function=app_launched_callback)
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 925, in launch_app
self.update_status(lambda response:
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 916, in update_status
callback_function=callback_function_param)
File "c:\python27\lib\site-packages\pychromecast\controllers_init_.py", line 84, in send_message
self.namespace, data, inc_session_id, callback_function)
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 721, in send_platform_message
inc_session_id, callback_function_param)
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 668, in send_message
self._ensure_channel_connected(destination_id)
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 758, in _ensure_channel_connected
no_add_request_id=True)
Exception in thread Thread-1:
Traceback (most recent call last):
File "c:\python27\lib\threading.py", line 801, in __bootstrap_inner
self.run()
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 423, in run
self.initialize_connection()
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 252, in initialize_connection
for service in self.services.copy():
AttributeError: 'list' object has no attribute 'copy'
File "c:\python27\lib\site-packages\pychromecast\socket_client.py", line 715, in send_message

str(self.port) + " is connecting...")

pychromecast.error.NotConnected: Chromecast 10.1.1.12:8009 is connecting...

@emontnemery emontnemery deleted the defer_connect branch April 3, 2019 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants