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

Allow dependency injection Config and Server #1248

Closed
wants to merge 10 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions tests/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -516,7 +516,10 @@ def test_ws_max_size() -> None:
def test_bind_unix_socket_works_with_reload_or_workers(tmp_path, reload, workers):
uds_file = tmp_path / "uvicorn.sock"
config = Config(
app=asgi_app, uds=uds_file.as_posix(), reload=reload, workers=workers
app="tests.test_config:asgi_app",
uds=uds_file.as_posix(),
reload=reload,
workers=workers,
)
config.load()
sock = config.bind_socket()
Expand All @@ -538,7 +541,12 @@ def test_bind_unix_socket_works_with_reload_or_workers(tmp_path, reload, workers
def test_bind_fd_works_with_reload_or_workers(reload, workers):
fdsock = socket.socket(socket.AF_UNIX, socket.SOCK_STREAM)
fd = fdsock.fileno()
config = Config(app=asgi_app, fd=fd, reload=reload, workers=workers)
config = Config(
app="tests.test_config:asgi_app",
fd=fd,
reload=reload,
workers=workers,
)
config.load()
sock = config.bind_socket()
assert isinstance(sock, socket.socket)
Expand Down
14 changes: 12 additions & 2 deletions tests/test_main.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,12 @@ async def test_run(host, url):

@pytest.mark.asyncio
async def test_run_multiprocess():
config = Config(app=app, loop="asyncio", workers=2, limit_max_requests=1)
config = Config(
app="tests.test_main:app",
loop="asyncio",
workers=2,
limit_max_requests=1,
)
async with run_server(config):
async with httpx.AsyncClient() as client:
response = await client.get("http://127.0.0.1:8000")
Expand All @@ -57,7 +62,12 @@ async def test_run_multiprocess():

@pytest.mark.asyncio
async def test_run_reload():
config = Config(app=app, loop="asyncio", reload=True, limit_max_requests=1)
config = Config(
app="tests.test_main:app",
loop="asyncio",
reload=True,
limit_max_requests=1,
)
async with run_server(config):
async with httpx.AsyncClient() as client:
response = await client.get("http://127.0.0.1:8000")
Expand Down
4 changes: 2 additions & 2 deletions uvicorn/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from uvicorn.config import Config
from uvicorn.main import Server, main, run
from uvicorn.main import Server, main, run, run_server

__version__ = "0.15.0"
__all__ = ["main", "run", "Config", "Server"]
__all__ = ["main", "run", "run_server", "Config", "Server"]
7 changes: 7 additions & 0 deletions uvicorn/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -444,6 +444,13 @@ def load(self) -> None:

self.lifespan_class = import_from_string(LIFESPAN[self.lifespan])

if (self.reload or self.workers > 1) and not isinstance(self.app, str):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this check go in the constructor? By the time we get here, we are already in the subprocess I believe...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this check should be combined with import_from_string(self.app).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? Isn't it preferable to "fail fast" in the main process rather than failing later in possibly N subprocesses?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the user provides a string, checks the type in one place, but tries to import it in another far away, don't you think this is unreasonable?

If it is for this purpose, then import_from_string(self.app) should also "fail fast".

logger.warning(
"You must pass the application as an import string to enable"
" 'reload' or 'workers'."
)
sys.exit(1)

try:
self.loaded_app = import_from_string(self.app)
except ImportFromStringError as exc:
Expand Down
10 changes: 3 additions & 7 deletions uvicorn/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -429,15 +429,11 @@ def main(
def run(app: typing.Union[ASGIApplication, str], **kwargs: typing.Any) -> None:
config = Config(app, **kwargs)
server = Server(config=config)
run_server(server)

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)

def run_server(server: Server) -> None:
config = server.config
if config.should_reload:
sock = config.bind_socket()
ChangeReload(config, target=server.run, sockets=[sock]).run()
Expand Down