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

fix websocket broadcasts in Tornado 5.x #1796

Merged
merged 2 commits into from Sep 9, 2019

Conversation

kingosticks
Copy link
Member

@kingosticks kingosticks commented Sep 8, 2019

In Tornado 4.x and earlier, IOLoop.current() would return the global IOLoop
if there was no IOLoop already running in the calling thread. This was the
case when calling our websocket handler's broadcast method from the HttpFrontend thread
and so callbacks were correctly scheduled on the HttpServer thread's IOLoop.
However, in Tornado 5.0+, the idea of a global IOLoop is gone and calling
IOLoop.current() will simply create a new IOLoop if there isn't one already
running in the calling thread.
This incorrectly resulted in callbacks being scheduled on that new IOLoop
which is never actually started and so the broadcasts were never sent.

To fix this we explicitly provide broadcast with the correct IOLoop to use (HttpServer's).

Also, tornado.websocket.websocket_connect removed the io_loop parameter, which we don't seem to need. I gave up trying to get the tests to fail under Tornado 5.x.

This is related to #1716.

In Tornado 4.x and earlier, IOLoop.current() would return the global IOLoop
if there was no IOLoop already running in the calling thread. This was the
case when calling our websocket broadcast method from the HttpFrontend thread
and so callbacks were correctly scheduled on the HttpServer thread's IOLoop.
However, in Tornado 5.0+, the idea of a global IOLoop is gone and calling
IOLoop.current() will simply create a new IOLoop if there isn't one already
running in the calling thread.
This incorrectly resulted in callbacks being scheduled on that new IOLoop
which is never actually started and so the broadcasts were never sent.

This is related to mopidy#1716.
@kingosticks kingosticks added the A-http label Sep 8, 2019
@kingosticks kingosticks self-assigned this Sep 8, 2019
@kingosticks kingosticks added this to the v3.0 milestone Sep 8, 2019
jodal
jodal approved these changes Sep 8, 2019
mopidy/http/actor.py Outdated Show resolved Hide resolved
mopidy/http/handlers.py Outdated Show resolved Hide resolved
tests/http/test_events.py Outdated Show resolved Hide resolved
@kingosticks
Copy link
Member Author

@kingosticks kingosticks commented Sep 8, 2019

Wow, the fact I managed to mess up that fixup and the tests still passed is really still a bit of an issue...

@jodal
Copy link
Member

@jodal jodal commented Sep 8, 2019

Not entirely with you. Is it anything that should be fixed before merging?

@kingosticks
Copy link
Member Author

@kingosticks kingosticks commented Sep 9, 2019

Nah, false alarm, it's good to go.

@kingosticks kingosticks merged commit 59a3935 into mopidy:develop Sep 9, 2019
1 check passed
@kingosticks kingosticks deleted the fix/tornado-5-support branch Sep 9, 2019
@kingosticks kingosticks mentioned this pull request Sep 9, 2019
3 tasks
@jodal jodal removed this from the v3.0 milestone Sep 30, 2019
@jodal jodal added this to the v2.3.0 milestone Sep 30, 2019
jodal pushed a commit that referenced this issue Sep 30, 2019
In Tornado 4.x and earlier, IOLoop.current() would return the global IOLoop
if there was no IOLoop already running in the calling thread. This was the
case when calling our websocket broadcast method from the HttpFrontend thread
and so callbacks were correctly scheduled on the HttpServer thread's IOLoop.
However, in Tornado 5.0+, the idea of a global IOLoop is gone and calling
IOLoop.current() will simply create a new IOLoop if there isn't one already
running in the calling thread.
This incorrectly resulted in callbacks being scheduled on that new IOLoop
which is never actually started and so the broadcasts were never sent.

This is related to #1716.

(cherry picked from commit 59a3935)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants