Since essentially the first use of Tornado in Zulip, we've been
maintaining our Tornado+Django system, AsyncDjangoHandler, with
several hundred lines of Django code copied into it.
The goal for that code was simple: We wanted a way to use our Django
middleware (for code sharing reasons) inside a Tornado process (since
we wanted to use Tornado for our async events system).
As part of the Django 2.2.x upgrade, I looked at upgrading this
implementation to be based off modern Django, and it's definitely
possible to do that:
* Continue forking load_middleware to save response middleware.
* Continue manually running the Django response middleware.
* Continue working out a hack involving copying all of _get_response
to change a couple lines allowing us our Tornado code to not
actually return the Django HttpResponse so we can long-poll. The
previous hack of returning None stopped being viable with the Django 2.2
MiddlewareMixin.__call__ implementation.
But I decided to take this opportunity to look at trying to avoid
copying material Django code, and there is a way to do it:
* Replace RespondAsynchronously with a response.asynchronous attribute
on the HttpResponse; this allows Django to run its normal plumbing
happily in a way that should be stable over time, and then we
proceed to discard the response inside the Tornado `get()` method to
implement long-polling. (Better yet might be raising an
exception?). This lets us eliminate maintaining a patched copy of
_get_response.
* Removing the @asynchronous decorator, which didn't add anything now
that we only have one API endpoint backend (with two frontend call
points) that could call into this. Combined with the last bullet,
this lets us remove a significant hack from our
never_cache_responses function.
* Calling the normal Django `get_response` method from zulip_finish
after creating a duplicate request to process, rather than writing
totally custom code to do that. This lets us eliminate maintaining
a patched copy of Django's load_middleware.
* Adding detailed comments explaining how this is supposed to work,
what problems we encounter, and how we solve various problems, which
is critical to being able to modify this code in the future.
A key advantage of these changes is that the exact same code should
work on Django 1.11, Django 2.2, and Django 3.x, because we're no
longer copying large blocks of core Django code and thus should be
much less vulnerable to refactors.
There may be a modest performance downside, in that we now run both
request and response middleware twice when longpolling (once for the
request we discard). We may be able to avoid the expensive part of
it, Zulip's own request/response middleware, with a bit of additional
custom code to save work for requests where we're planning to discard
the response. Profiling will be important to understanding what's
worth doing here.
Zulip has had a small use of WebSockets (specifically, for the code
path of sending messages, via the webapp only) since ~2013. We
originally added this use of WebSockets in the hope that the latency
benefits of doing so would allow us to avoid implementing a markdown
local echo; they were not. Further, HTTP/2 may have eliminated the
latency difference we hoped to exploit by using WebSockets in any
case.
While we’d originally imagined using WebSockets for other endpoints,
there was never a good justification for moving more components to the
WebSockets system.
This WebSockets code path had a lot of downsides/complexity,
including:
* The messy hack involving constructing an emulated request object to
hook into doing Django requests.
* The `message_senders` queue processor system, which increases RAM
needs and must be provisioned independently from the rest of the
server).
* A duplicate check_send_receive_time Nagios test specific to
WebSockets.
* The requirement for users to have their firewalls/NATs allow
WebSocket connections, and a setting to disable them for networks
where WebSockets don’t work.
* Dependencies on the SockJS family of libraries, which has at times
been poorly maintained, and periodically throws random JavaScript
exceptions in our production environments without a deep enough
traceback to effectively investigate.
* A total of about 1600 lines of our code related to the feature.
* Increased load on the Tornado system, especially around a Zulip
server restart, and especially for large installations like
zulipchat.com, resulting in extra delay before messages can be sent
again.
As detailed in
https://github.com/zulip/zulip/pull/12862#issuecomment-536152397, it
appears that removing WebSockets moderately increases the time it
takes for the `send_message` API query to return from the server, but
does not significantly change the time between when a message is sent
and when it is received by clients. We don’t understand the reason
for that change (suggesting the possibility of a measurement error),
and even if it is a real change, we consider that potential small
latency regression to be acceptable.
If we later want WebSockets, we’ll likely want to just use Django
Channels.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Apparently, our change in b8a1050fc4 to
stop caching responses on API endpoints accidentally ended up
affecting uploaded files as well.
Fix this by explicitly setting a Cache-Control header in our Sendfile
responses, as well as changing our outer API caching code to only set
the never cache headers if the view function didn't explicitly specify
them itself.
This is not directly related to #13088, as that is a similar issue
with the S3 backend.
Thanks to Gert Burger for the report.
There’s an apparent contradiction between RFC 7230 §3.3.2
Content-Length:
“A server MUST NOT send a Content-Length header field in any response
with a status code of 1xx (Informational) or 204 (No Content).”
and RFC 7231 §4.3.7 OPTIONS:
“A server MUST generate a Content-Length field with a value of "0" if
no payload body is to be sent in the response.”
The only resolution within the existing language would be to disallow
all 204 responses to OPTIONS requests. However, I don’t think that
was the intention, so I submitted this erratum report:
https://www.rfc-editor.org/errata/eid5806
and updated the code accordingly.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Previous cleanups (mostly the removals of Python __future__ imports)
were done in a way that introduced leading newlines. Delete leading
newlines from all files, except static/assets/zulip-emoji/NOTICE,
which is a verbatim copy of the Apache 2.0 license.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Investigation into #12876, a mysterious bug where users were seeing
messages reappear as unread, determined that the root cause was
missing headers to disable client-side caching for Zulip's REST API
endpoints.
This manifested, in particular, for `GET /messages`, which is
essentially the only API GET endpoint used by the webapp at all. When
using the `Ctrl+Shift+T` feature of browsers to restore a recently
closed tab (and potentially other code paths), the browser would
return from its disk cache a cached copy of the GET /messages results.
Because we include message flags on messages fetched from the server,
this in particular meant that those tabs would get a stale version of
the unread flag for the batches of the most recent ~1200 messages that
Zulip fetches upon opening a new browser tab.
The issue took same care to reproduce as well, in large part because
the arguments to those initial GET /messages requests will vary as one
reads messages (because the `pointer` moves forward) and then enters
the "All messages" view; the disk cache is only used for GET requests
with the exact same URL parameters.
We will probably still want to merge the events error-handling changes
we had previously proposed for this, but the conclusion of this being
a straightforward case of missing cache-control headers is much more
satisfying than the "badly behaving Chrome" theory discussed in the
issue thread.
Fixes#12876.
This should make it possible for blueslip error reports to be sent on
our logged-out portico pages, which should in turn make it possible to
debug any such issues as they occur.
The code paths for accessing user-uploaded files are both (A) highly
optimized so as to not require a ton of work, and (B) a code path
where it's totally reasonable for a client to need to fetch 100+
images all at once (e.g. if it's the first browser open in a setting
with a lot of distinct senders with avatars or a lot of image
previews).
Additionally, we've been seeing exceptions logged in the production
redis configuration caused by this code path (basically, locking
failures trying to update the rate-limit data structures).
So we skip running our current rate limiting algorithm for these views.
This extends the /user_uploads API endpoint to support passing the
authentication credentials via the URL, not the HTTP_AUTHORIZATION
headers. This is an important workaround for the fact that React
Native's Webview system doesn't support setting HTTP_AUTHORIZATION;
the app will be responsible for rewriting URLs for uploaded files
directly to add this parameter.
This fixes a regression in 93678e89cd
and a4979410f9, where the webhooks using
authenticated_rest_api_view were migrated to a new model that didn't
include setting a custom Client string for the webhook.
When restoring these webhooks' client strings, we also fix places
where the client string was not capitalized the same was as the
product's name.
For a long time, rest_dispatch has had this hack where we have to
create a copy of it in each views file using it, in order to directly
access the globals list in that file. This removes that hack, instead
making rest_dispatch just use Django's import_string to access the
target method to use.
[tweaked and reorganized from acrefoot's original branch in various
ways by tabbott]
If a user's session cookie expired, the next REST API request their
browser did would go into the json_unauthorized code path. This
returned a response with a WWW-Authenticate tag for HTTP Basic Auth
(since that's what the REST API uses), even for /json requests which
should only be authenticated using session auth.
We fix this by explicitly passing the desired WWW-Authenticate state.
Fixes: #800.
This caused problems with our tests suite where we were using a logged
in browser session and actually acting as a different user.
(imported from commit 73b8cb39d5d669e682fbacf2f7e574c228885c2f)
If you don't have a cookie or basic auth and the request looks like
a top-level page in the browser, redirect to the login page.
(imported from commit fc1bcb1080591522bd1b694664255f7049a5d443)
Anders Kaseorg
Anders Kaseorg
Tim Abbott
Anders Kaseorg
rht
Umair Khan
Aditya Bansal
bulat22101
rahuldeve
acrefoot
Tim Abbott
Tomasz Kolek
David Adamec
Luke Faraone
Tim Abbott
Steve Howell
Kevin Mehall