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

Iterate over copy of websocket clients #1993

Merged
merged 1 commit into from Jul 23, 2021

Conversation

jasminhacker
Copy link
Contributor

If clients are added/removed while a message is broadcast, a python error is raised:

Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 193, in _actor_loop
    response = self._handle_receive(envelope.message)
  File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 299, in _handle_receive
    return callee(*message.args, **message.kwargs)
  File "/usr/lib/python3/dist-packages/mopidy/http/actor.py", line 82, in on_event
    on_event(name, self.server.io_loop, **data)
  File "/usr/lib/python3/dist-packages/mopidy/http/actor.py", line 89, in on_event
    handlers.WebSocketHandler.broadcast(message, io_loop)
  File "/usr/lib/python3/dist-packages/mopidy/http/handlers.py", line 112, in broadcast
    for client in cls.clients:
RuntimeError: Set changed size during iteration

For me this happens when testing a project that uses mopidyapi. During testing, websocket connections are frequently created/removed, which sometimes leads to this error. Code to reproduce:

from mopidyapi import MopidyAPI
# for me the error started showing up with ~4 iterations,
# but more make it appear more frequently
for _ in range(8):
    api = MopidyAPI()
    api.playback.pause()

This pull request prevents this by iterating over a copy of the clients instead of the original set. A shallow copy should be sufficient, as the clients are only added/removed from the set and not changed. In my use-case this caused no problems.

If clients are added/removed while a message is broadcast, a python error is raised: 
```
Traceback (most recent call last):
  File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 193, in _actor_loop
    response = self._handle_receive(envelope.message)
  File "/usr/lib/python3/dist-packages/pykka/_actor.py", line 299, in _handle_receive
    return callee(*message.args, **message.kwargs)
  File "/usr/lib/python3/dist-packages/mopidy/http/actor.py", line 82, in on_event
    on_event(name, self.server.io_loop, **data)
  File "/usr/lib/python3/dist-packages/mopidy/http/actor.py", line 89, in on_event
    handlers.WebSocketHandler.broadcast(message, io_loop)
  File "/usr/lib/python3/dist-packages/mopidy/http/handlers.py", line 112, in broadcast
    for client in cls.clients:
RuntimeError: Set changed size during iteration
```

For me this happens when testing a project that uses [mopidyapi](https://github.com/AsbjornOlling/mopidyapi/). During testing, websocket connections are frequently created/removed, which sometimes leads to this error. Code to reproduce:
```python
from mopidyapi import MopidyAPI
# for me the error started showing up with ~4 iterations,
# but more make it appear more frequently
for _ in range(8):
    api = MopidyAPI()
    api.playback.pause()
```

This pull request prevents this by iterating over a copy of the clients instead of the original set. A shallow copy should be sufficient, as the clients are only added/removed from the set and not changed. In my use-case this caused no problems.
@kingosticks
Copy link
Member

There might be an argument that MopidyAPI should provide a way to close it's connections properly but that would only help in contrived cases like this. But either way your fix looks sensible. Could you add a brief comment or something in the commit messages which explains why we are doing this? Do we need a test for this?

@jasminhacker
Copy link
Contributor Author

Should I shorten the commit message? It already contains my first comment from this pull request.

If you want me to write a test I could look into it. Although I'm not sure if I could write a test that reliably provokes this error. If the test works in the unmodified version as well, it is not very meaningful.

@kingosticks
Copy link
Member

Sorry, the commit message didn't show up at all in the stupid github app I have on my phone.

@kingosticks kingosticks added A-http Area: HTTP frontend C-bug Category: This is a bug needs changelog entry labels Jul 23, 2021
@kingosticks kingosticks added this to the Next bug fix release milestone Jul 23, 2021
@kingosticks kingosticks merged commit fd92546 into mopidy:develop Jul 23, 2021
@kingosticks
Copy link
Member

Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http Area: HTTP frontend C-bug Category: This is a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants