Ensure non-zero exit code when startup fails

[mboutet]

Fixes #780

I think this is a pretty straightforward and minimal fix. I tested it in my app and it works.

I'm using uvicorn like this:

uvicorn \
  --host "0.0.0.0" \
  --port "${SERVER_PORT}" \
  --access-log \
  --log-level debug \
  --limit-concurrency "${UVICORN_LIMIT_CONCURRENCY:-30}" \
  --backlog "${UVICORN_BACKLOG:-2048}" \
  myapp.main:app

In other words, I don't use a supervisor or something like that, so I'm not sure if those use cases are covered.

Let me know what you think.

[hongquan]

Could you return the exit code which follow some existing conventions? https://github.com/sixty-north/exit-codes/blob/master/exit_codes/exit_codes.py#L87

In this case, I think the code 78 (configuration error) is relevant.

[mboutet]

Personally, I don't really mind using another non-zero code other than 1. However, it would be inconsistent with other parts of the codebase such as:

uvicorn/uvicorn/main.py

Lines 373 to 379 in d78394e

	if (config.reload or config.workers > 1) and not isinstance(app, str):
	logger = logging.getLogger("uvicorn.error")
	logger.warning(
	"You must pass the application as an import string to enable 'reload' or "
	"'workers'."
	)
	sys.exit(1)

To be consistent, this exit code should also be 78 and I don't think that changing this would be a good idea since some users might rely on this behaviour. If you search sys.exit( in the codebase, we can see that exit code 1 is always used, mainly in cases where some kind of configuration error happened (i.e. before the server startup).

Let's wait for other opinions, and I'll change it if there's a consensus.

[liketechnik]

I've personally had a similar, although different issue, where an exception in my startup handler did not cause uvicorn to return an exit code != 0.

Therefore the current fix would not only catch configuration errors, but also application level errors. So either the handling of startup errors needs to be more fine grained or it is impossible to report the specific error cause via the exit code, imo.

[pquentin]

@florimondmanca @euri10 What are your thoughts on this? I got confused today again by this: I thought my app was shutdown externally when the issue was in fact an internal startup failure in my own code.

[euri10]

yes this should be definitely be dealt with, I'm not sure the 1 exit is the most sensible way though,
and I dont have the time right now but it's on the todo !

[florimondmanca]

Could someone interested in moving this forward share some before/after output for Uvicorn's behavior?

Edit: Actually I understand the issue is that the output shows that there was an issue on startup (and Uvicorn shuts down accordingly), but exit code is currently still zero in that case, right?

Is there anything we need to consider for the GunicornWorker? Does that handle startup/shutdown events too?

This might cause some breakage in case people falsely rely on "error on startup exits as OK", so let's just make sure to release this in a minor bump.

[pquentin]

Edit: Actually I understand the issue is that the output shows that there was an issue on startup (and Uvicorn shuts down accordingly), but exit code is currently still zero in that case, right?

Right!

I don't know about GunicornWorker, sorry.

[mboutet]

@florimondmanca, I'm using the uvicorn.workers.UvicornWorker with gunicorn and the exit code is nonzero when there's a failure during startup if that's what you're asking.

[euri10]

Long time I haven't thought about his but what would be the difference with a raise click.Abort() here ? I know we're using sys.exit(1) everywhere else instead of this pattern in the codebase, I just wondered what difference this could make, if that could be an improvement over just sending exit ?

Also this covers the only case where uvicorn is ran without --relaod or --workers , this would need to be considered as well in fixing the issue.

And finally the gunicorn case, but I think you alreaduy answered that

All that to say also a test for this would be awesome.

[Kludex]

Gunicorn exits with code 3 if boot (startup) fails, and we accepted #1077.
I believe, for practicality, uvicorn should also exit with code 3.

Would you guys agree? @florimondmanca @euri10

[Kludex]

I'd like to speed up this, and I don't agree with the exit code. Let's continue on #1278.