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

Fix WS disconnect race #1020

Merged
merged 3 commits into from Mar 1, 2015

Conversation

2 participants
@adamcik
Member

adamcik commented Mar 1, 2015

No description provided.

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Darn, seems I need to do some magic for cross version testing of this with respect to tornado.

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Ok, all good now.

yield self.connection()
# Tornado checks for ws_connection and raises WebSocketClosedError
# if it is missing, this test case simulates winning a race were
# this has happened but we have not yet been remove from clients.

This comment has been minimized.

@jodal

jodal Mar 1, 2015

Member

s/remove/removed/

@jodal

This comment has been minimized.

Member

jodal commented Mar 1, 2015

Looks good to me.

What did this bug look like? A crashed handler in Mopidy and other handlers and new handlers continued working?

@jodal jodal added this to the v0.20 - Audio cleanup 1 milestone Mar 1, 2015

@jodal jodal self-assigned this Mar 1, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Not quite sure, I killed it before I realized what had happened :(

adamcik added some commits Mar 1, 2015

http: Make WS broadcast more robust against disconnect race
Adds some WebSocketHandler tests that actually connect using a WS client and
plugs a potential race condition.

Any call to write_message could fail, either due to WebSocketClosedError like
in the log below, or simply due to socket errors. To play it safe we catch all
errors and debug log that a broadcast failed.

2015-02-26 21:24:02,266 ERROR    [HttpServer] /home/adamcik/dev/mopidy/mopidy/http/handlers.py:116
  mopidy.http.handlers WebSocket request error: deque index out of range
2015-02-26 21:24:10,098 ERROR    [HttpFrontend-11] build/bdist.linux-x86_64/egg/pykka/actor.py:268
  pykka Unhandled exception in HttpFrontend (urn:uuid:e376bd95-c32e-4e17-ad20-7d0b3c0cf2b2):
Traceback (most recent call last):
  File "build/bdist.linux-x86_64/egg/pykka/actor.py", line 200, in _actor_loop
    response = self._handle_receive(message)
  File "build/bdist.linux-x86_64/egg/pykka/actor.py", line 294, in _handle_receive
    return callee(*message['args'], **message['kwargs'])
  File ".../dev/mopidy/mopidy/http/actor.py", line 77, in on_event
    on_event(name, **data)
  File ".../dev/mopidy/mopidy/http/actor.py", line 84, in on_event
    handlers.WebSocketHandler.broadcast(message)
  File ".../dev/mopidy/mopidy/http/handlers.py", line 78, in broadcast
    client.write_message(msg)
  File ".../dev/mopidy-virtualenv/local/lib/python2.7/site-packages/tornado/websocket.py", line 183, in write_message
    raise WebSocketClosedError()
WebSocketClosedError

@adamcik adamcik force-pushed the adamcik:feature/fix-ws-disconnect-race branch from 3648318 to 6c5970f Mar 1, 2015

@adamcik

This comment has been minimized.

Member

adamcik commented Mar 1, 2015

Typo fixed with a rebase this time round.

jodal added a commit that referenced this pull request Mar 1, 2015

@jodal jodal merged commit 0634de6 into mopidy:develop Mar 1, 2015

2 checks passed

Scrutinizer 7 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jodal jodal added the 3 - Done label Mar 1, 2015

@adamcik adamcik deleted the adamcik:feature/fix-ws-disconnect-race branch Mar 2, 2015

@jodal jodal removed the 3 - Done label Mar 11, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment