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

http: use current Tornado IOLoop when stopping. (Fixes #1715). #1716

Merged
merged 1 commit into from Oct 22, 2018

Conversation

kingosticks
Copy link
Member

@kingosticks kingosticks commented Oct 20, 2018

From Tornado v5.0:

IOLoop.instance is now a deprecated alias for IOLoop.current. Applications that need the cross-thread communication behavior facilitated by IOLoop.instance should use their own global variable instead.

We were using IOLoop.instance to stop the HTTP server thread from a different thread.

@kingosticks kingosticks force-pushed the fix/hang-on-stopping-http branch from a22ba85 to d6a5f82 Compare Oct 20, 2018
@kingosticks kingosticks requested a review from jodal Oct 20, 2018
jodal
jodal approved these changes Oct 21, 2018
Copy link
Member

@jodal jodal left a comment

I assume this works for Tornado 4.4 and upwards?

@kingosticks
Copy link
Member Author

@kingosticks kingosticks commented Oct 21, 2018

Yep I did a few tests on both tornado 5 and 4.4, seems fine.

@jodal
Copy link
Member

@jodal jodal commented Oct 21, 2018

Maybe target this for the release-2.2 branch? And add a changelog entry.

This is in response to the breaking change in Tornado v5.0 where
IOLoop.instance is now a deprecated alias for IOLoop.current.
More info at https://www.tornadoweb.org/en/stable/releases/v5.0.0.html
@kingosticks kingosticks force-pushed the fix/hang-on-stopping-http branch from d6a5f82 to 1d30e73 Compare Oct 22, 2018
@kingosticks kingosticks changed the base branch from develop to release-2.2 Oct 22, 2018
@jodal jodal added this to the v2.2.2 milestone Oct 22, 2018
@jodal jodal added the A-http label Oct 22, 2018
@jodal jodal merged commit 10c571b into mopidy:release-2.2 Oct 22, 2018
1 check was pending
kingosticks added a commit to kingosticks/mopidy that referenced this issue 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 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 added a commit that referenced this issue Sep 9, 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.
@kingosticks kingosticks mentioned this pull request Sep 9, 2019
3 tasks
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