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

get_chromcasts() copy device list before iteration #489

Merged
merged 1 commit into from
Apr 15, 2021

Conversation

Shane-Wiseman-Bose
Copy link
Contributor

@Shane-Wiseman-Bose Shane-Wiseman-Bose commented Apr 12, 2021

resolves #488 by making a copy of device list returned from discovery before iterating over list.

@Shane-Wiseman-Bose
Copy link
Contributor Author

Shane-Wiseman-Bose commented Apr 12, 2021

There is another option of making the copy on the iterable return from discover_chromecasts() which can protect all calls. I figured to lessen the scope for ease of review and to reflect my knowledge depth of the repos.

return (browser.devices.values(), browser)

@Shane-Wiseman-Bose
Copy link
Contributor Author

Example of issue.

>>> a = {"a": 1, "b": 2}
>>> b = a.values()
>>> for i in b:
...     if "a" in a:
...             del(a["a"])
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
RuntimeError: dictionary changed size during iteration

Example of Fix

>>> a = {"a": 1, "b": 2}
>>> b = a.values()
>>> for i in list(b):
...     if "a" in a:
...             del(a["a"])
...
>>>

@emontnemery
Copy link
Collaborator

There is another option of making the copy on the iterable return from discover_chromecasts() which can protect all calls

That's a better option, can you update the PR to do that instead?

…s not thread safe

resolves home-assistant-libs#488 by making a copy of device list return from discovery before iterating over list.
@Shane-Wiseman-Bose
Copy link
Contributor Author

moved the list copy to discovery.py calls. A user can still access the mutable device object by referencing the browser attribute browser.devices

@Shane-Wiseman-Bose
Copy link
Contributor Author

@emontnemery is there anything else pending on this?

@emontnemery
Copy link
Collaborator

@Shane-Wiseman-Bose nothing else, thanks for the PR! 👍

@emontnemery emontnemery merged commit bd0cfe5 into home-assistant-libs:master Apr 15, 2021
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.

RuntimeError - get_chromecasts()
2 participants