In addition to being generally more correct, this works around a bug
in Node.js that causes webpack-dev-server to corrupt the terminal
state when exiting as a background process.
https://github.com/nodejs/node/issues/35536
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Now that all casper tests have been migrated to
puppeteer, there's no need for having casper
related things.
Removed the casperjs package and removed/replaced
casper in few places with puppeteer.
Only removed few of them which I'm confident
about. Also didn't make any changes in docs
as it would be easier to remove them while
adding puppeteer docs.
Use read-only types (List ↦ Sequence, Dict ↦ Mapping, Set ↦
AbstractSet) to guard against accidental mutation of the default
value.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Fixes#2665.
Regenerated by tabbott with `lint --fix` after a rebase and change in
parameters.
Note from tabbott: In a few cases, this converts technical debt in the
form of unsorted imports into different technical debt in the form of
our largest files having very long, ugly import sequences at the
start. I expect this change will increase pressure for us to split
those files, which isn't a bad thing.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Automatically generated by the following script, based on the output
of lint with flake8-comma:
import re
import sys
last_filename = None
last_row = None
lines = []
for msg in sys.stdin:
m = re.match(
r"\x1b\[35mflake8 \|\x1b\[0m \x1b\[1;31m(.+):(\d+):(\d+): (\w+)", msg
)
if m:
filename, row_str, col_str, err = m.groups()
row, col = int(row_str), int(col_str)
if filename == last_filename:
assert last_row != row
else:
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
with open(filename) as f:
lines = f.readlines()
last_filename = filename
last_row = row
line = lines[row - 1]
if err in ["C812", "C815"]:
lines[row - 1] = line[: col - 1] + "," + line[col - 1 :]
elif err in ["C819"]:
assert line[col - 2] == ","
lines[row - 1] = line[: col - 2] + line[col - 1 :].lstrip(" ")
if last_filename is not None:
with open(last_filename, "w") as f:
f.writelines(lines)
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
For basic testing (either manual or automated), we
generally only need the server and tornado running.
Obviously, it's nice to test the complete system,
but if you're on a slow PC, the overhead can be
annoying.
Note that we don't launch any of these processes
in `--streamlined` mode:
process_queue
process_fts_updates
deliver_scheduled_messages
thumbor
And then by not launching process_queue, we avoid
several child processes.
Basic functionality like sending messages will
still work here.
The streamlined mode may be helpful in debugging
our generally slow server startup time. Obviously,
some of the problem with startup is the auxiliary
processes here, but removing them as a variable
could help us focus on getting the core stuff fast.
Note that we still have the webpack watcher running
in streamlined mode.
For the particular case of thumbor, note that we
modify the proxy server to explicitly print and
return an error if we get a `/thumbor/*` request.
We clean up the code related to launching
processes here.
We extract:
server_processes
We also extract these helper for webpack
stuff:
do_one_time_webpack_compile
start_webpack_watcher
And then we move the code to actually launch
them lexically within the file (so as not to
be obscured by various function definitions).
Here is the new output for displaying ports:
Zulip services will listen on ports:
9991: web proxy
9992: Django
9993: Tornado
9994: webpack
9995: Thumbor
Note to Vagrant users: Only the proxy port (9991) is exposed.
I tone down the yellow for the Vagrant warning, and I show
the web proxy in cyan to emphasize it.
I also extracted the code into a function, and I don't call
that function until after `app.listen()`. (The users probably
won't notice much difference in the timing of this message, but
the message won't show if the `listen` step fails for some
reason, which I think is what we want here.)
We remove the import-tools code that was plunked
right into the middle of our command line
arguments.
Then we add a local var called `DESCRIPTION` to
fix some ugly code formatting, and we stop with the
unnecessary `r` prefix to the multi-line string.
We recently changed our droplet setup such that their
host names no longer include zulipdev.org. This caused
a few things to break.
The particular symptom that this commit fixes is that
we were trying to server static assets from
showell:9991 instead of showell.zulipdev.org:9991,
which meant that you couldn't use the app locally.
(The server would start, but the site's pretty unusable
without static assets.)
Now we rely 100% on `dev_settings.py` to set
`EXTERNAL_HOST` for any droplet users who don't set
that var in their own environment. That allows us to
remove some essentially duplicate code in `run-dev.py`.
We also set `IS_DEV_DROPLET` explicitly, so that other
code doesn't have to make inferences or duplicate
logic to detemine whether we're a droplet or not.
And then in `settings.py` we use `IS_DEV_DROPLET` to
know that we can use a prod-like method of calculating
`STATIC_URL`, instead of hard coding `localhost`.
We may want to iterate on this further--this was
sort of a quick fix to get droplets functional again.
It's possible we can re-configure droplets to have
folks get reasonable `EXTERNAL_HOST` settings in their
bash profiles, or something like that, although that
may have its own tradeoffs.
This reverts commit 36a8e61e67 (#13934).
The Django 2.2 autoreloader works by forking into a child process that
exits with status 3 when a file changes, and a parent process that
restarts the child when it exits with status 3. Setting this
environment variable had the effect of pretending we were already the
child process, without a parent process to restart it. Therefore,
changing any code used by the queue processor caused it to exit rather
than restart.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
In Django 2.2 the autoreload system has changed.
DJANGO_AUTORELOAD_ENV env variable should be set when calling code
that'll use the autoreloader. Otherwise there's some kind of race
condition in the autoreload code when SIGINT is sent, where
restart_with_reloader() (called only if the env variable isn't set)
has the subprocess module calling p.kill() on a process that's already
exited, raising ProcessLookupError and printing an ugly traceback. This
causes non-deterministic test-run-dev failures.
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>
Previously, while Django code that relied on EXTERNAL_HOST and other
settings would know the Zulip server is actually on port 9991, the
upcoming Django SAML code in python-social-auth would end up detecting
a port of 9992 (the one the Django server is actually listening on).
We fix this using X-Forwarded-Port.
Apparently Tornado decompresses gzip responses by default. Worse, it
fails to adjust the Content-Length header when it does.
https://github.com/tornadoweb/tornado/issues/2743
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
A HEAD response has a Content-Length but no body; it’s not correct in
that case to let Tornado default Content-Length to 0.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
As of #367, `tools/run-dev-queue-processors` has evolved into nothing
more than an unnecessarily elaborate wrapper around `manage.py
process_queue --all`. Remove it (mostly to make it marginally easier
to Tab-complete `tools/run-dev.py`, if I’m being honest).
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This seems to be a common enough pitfall to justify
a bit of extra handling. Example output:
$ ./tools/run-dev.py
Clearing memcached ...
Flushing memcached...
OK
Starting Zulip services on ports: web proxy: ...
Note: only port 9991 is exposed to the host in a Vagrant environment.
ERROR: You probably have another server running!!!
Traceback (most recent call last):
File "./tools/run-dev.py", line 421, in <module>
app.listen(proxy_port, address=options.interface)
File "/srv/zulip-py3-venv/lib/python3.5/...
server.listen(port, address)
File "/srv/zulip-py3-venv/lib/python3.5/...
sockets = bind_sockets(port, address=address)
File "/srv/zulip-py3-venv/lib/python3.5/...
sock.bind(sockaddr)
OSError: [Errno 98] Address already in use
Terminated
Before this change, the way we loaded
webpack for various tools was brittle.
First, I addressed test-api and test-help-documentation.
These tools used to be unable to run standalone on a
clean provision, because they were (indirectly)
calling tools/webpack without the `--test` option.
The problem was a bit obscure, since running things
like `./tools/test-backend` or `./tools/test-all` in
your workflow would create `./var/webpack-stats-test.json`
for the broken tools (and then they would work).
The tools themselves weren't broken; they were the
only relying on the common `test_server_running` helper.
And even that helper wasn't broken; it was just that
`run-dev.py` wasn't respecting the `--test` option.
So I made it so that `./tools/run-dev` passes in `--test` to
`./tools/webpack`.
To confuse matters even more, for some reason Casper
uses `./webpack-stats-production.json` via various
hacks for its webpack configuration, so when I fixed
the other tests, it broke Casper.
Here is the Casper-related hack in zproject/test_settings.py,
which was in place before my change and remains
after it:
if CASPER_TESTS:
WEBPACK_FILE = 'webpack-stats-production.json'
else:
WEBPACK_FILE = os.path.join('var', 'webpack-stats-test.json')
I added similar logic in tools/webpack:
if "CASPER_TESTS" in os.environ:
build_for_prod_or_casper(args.quiet)
I also made the helper functions in `./tools/webpack` have
nicer names.
So, now tools should all be able to run standalone and not
rely on previous tools creating webpack stats files for
them and leaving them in the file system. That's good.
Things are still a bit janky, though. It's not completely
clear to me why `test-js-with-casper` should work off of
a different webpack configuration than the other tests.
For now most of the jankiness is around Casper, and we have
hacks in two different places, `zproject/test_settings.py` and
`tools/webpack` to force it to use the production stats
file instead of the "test" one, even though Casper uses
test-like settings for other things like which database
you're using.
This changes run-dev.py to ensure that we have in fact compiled
handlebars templates before running webpack, which is the right model.
Future work will likely include running the handlebars compiler from
webpack, and thus eliminating this extra process.
This commit adds a --quiet argument to tools/webpack which removes
the verbose output from webpack and replaces it with showing only
errors. It also makes tools/run-dev --tests use this argument while
running webpack for testing.
Tweaked by tabbott to clean up the code a bit.
Webpack dev server by default does host checking for requests. so
in dev enviorment if the the request came for zulipdev.com it would not
send js files which caused dev envoirment to not work.
For now, this does nothing in a production environment, but it should
simplify the process of doing testing on the Thumbor implementation,
by integrating a lot of dependency management logic.
This reverts commit 66261f1cc. See parent commit for reason; here,
provision worked but `tools/run-dev.py` would give errors.
We need to figure out a test that reproduces these issues, then make a
version of these changes that keeps that test working, before we
re-merge them.
Anders Kaseorg
Riken Shah
Ganesh Pawar
Tim Abbott
Dinesh
Anders Kaseorg
Tim Abbott
Steve Howell
Mateusz Mandera
Wyatt Hoodes
Thomas Ip
Aman Agrawal
Anders Kaseorg
Armaan Ahluwalia
Priyank Patel
Aditya Bansal
Greg Price