Upgrade to Tornado 6 #21469
Merged
Upgrade to Tornado 6 #21469
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zulipbot
added
size: XL
area: dependencies
difficult
|
Hello @zulip/server-dependencies members, this pull request was labeled with the "area: dependencies" label, so you may want to check it out! |
|
All the changes here lgtm aside from settling some plan for telling whether a Tornado process is close to being overloaded. |
|
This does seem to work in production and development, but still needs test suite wrangling. |
andersk
force-pushed
the
tornado
branch
4 times, most recently
from
95a4d84
to
fdfce23
Compare
|
Hmm. This breaks heartbeat events somehow. |
|
Fixed that bug, I think, and added a safeguard to prevent the kind of accidental creation of extra event loops that triggered it. |
andersk
force-pushed
the
tornado
branch
3 times, most recently
from
3a57374
to
b41e2cd
Compare
andersk
force-pushed
the
tornado
branch
2 times, most recently
from
1b30aaa
to
f4b4ac9
Compare
andersk
force-pushed
the
tornado
branch
3 times, most recently
from
3c04322
to
b7cac58
Compare
|
Test-deploying this updated version on chat.zulip.org. |
|
(Updated the asgiref patch to the commit merged upstream in django/asgiref#320. No relevant differences; it was just rebased on top of some test suite changes.) |
andersk
force-pushed
the
tornado
branch
3 times, most recently
from
30fae99
to
eebeba5
Compare
Signed-off-by: Anders Kaseorg <anders@zulip.com>
We previously forked tornado.autoreload to work around a problem where it would crash if you introduce a syntax error and not recover if you fix it (tornadoweb/tornado#2398). A much more maintainable workaround for that issue, at least in current Tornado, is to use tornado.autoreload as the main module. Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
IOLoop.set_blocking_log_threshold and IOLoop.handle_callback_exception are removed in Tornado 6. Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
According to the documentation: “Pika does not have any notion of threading in the code. If you want to use Pika with threading, make sure you have a Pika connection per thread, created in that thread. It is not safe to share one Pika connection across threads, with one exception: you may call the connection method add_callback_threadsafe from another thread to schedule a callback within an active pika connection.” https://pika.readthedocs.io/en/stable/faq.html This also means that synchronous Django code running in Tornado will use its own synchronous SimpleQueueClient rather than sharing the asynchronous TornadoQueueClient, which is unfortunate but necessary as they’re about to be on different threads. Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Fixes zulip#8913. Signed-off-by: Anders Kaseorg <anders@zulip.com>
|
Thank for all of this cleanup, @andersk! Excited to have removed this crusty old version pin. |
Closed
andersk
added a commit
to andersk/zulip
that referenced
this pull request
Jun 8, 2022
Commit 6fd1a55 (zulip#21469) introduced an await point where get_events_backend calls fetch_events in order to switch threads. This opened the possibility that, in the window between the connect_handler call in fetch_events and the old location of this assignment in get_events_backend, an event could arrive, causing ClientDescriptor.add_event to crash on missing handler._request. Fix this by assigning handler._request earlier. Signed-off-by: Anders Kaseorg <anders@zulip.com>
timabbott
pushed a commit
that referenced
this pull request
Jun 9, 2022
Commit 6fd1a55 (#21469) introduced an await point where get_events_backend calls fetch_events in order to switch threads. This opened the possibility that, in the window between the connect_handler call in fetch_events and the old location of this assignment in get_events_backend, an event could arrive, causing ClientDescriptor.add_event to crash on missing handler._request. Fix this by assigning handler._request earlier. Signed-off-by: Anders Kaseorg <anders@zulip.com>
andersk
added a commit
to andersk/zulip
that referenced
this pull request
Jul 6, 2022
Commit 6fd1a55 (zulip#21469) introduced an await point where get_events_backend calls fetch_events in order to switch threads. This opened the possibility that, in the window between the connect_handler call in fetch_events and the old location of this assignment in get_events_backend, an event could arrive, causing ClientDescriptor.add_event to crash on missing handler._request. Fix this by assigning handler._request earlier. Signed-off-by: Anders Kaseorg <anders@zulip.com> (cherry picked from commit e112b61)
timabbott
pushed a commit
that referenced
this pull request
Jul 7, 2022
Commit 6fd1a55 (#21469) introduced an await point where get_events_backend calls fetch_events in order to switch threads. This opened the possibility that, in the window between the connect_handler call in fetch_events and the old location of this assignment in get_events_backend, an event could arrive, causing ClientDescriptor.add_event to crash on missing handler._request. Fix this by assigning handler._request earlier. Signed-off-by: Anders Kaseorg <anders@zulip.com> (cherry picked from commit e112b61)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This removes the
instrument_tornado_iolooplogging hack that cannot be ported past Tornado 4. We should replace it with a simpler metric, such as the fraction of time we spend blocked onbasic_consumewaiting for a message, or just the average length of the queues.