Bind serve's socket before writing client.json
A serve that lost the port race had already signed in, so client.json named a port another engine held and every client talked to that one. The socket is now bound first and handed to uvicorn.
This commit is contained in:
+36
-12
@@ -495,8 +495,7 @@ def cmd_serve(args: argparse.Namespace) -> int:
|
||||
|
||||
data_dir = _data_dir(args.data_dir, args.shared)
|
||||
# Before `_prepare`, so a refusal never migrates the database another
|
||||
# engine is serving out of, and before `_sign_in`, so the credential keeps
|
||||
# pointing at the engine that is actually up.
|
||||
# engine is serving out of.
|
||||
where = already_serving(data_dir, args.host)
|
||||
if where:
|
||||
_say(f"An engine for {data_dir} is already serving at {where}.")
|
||||
@@ -565,6 +564,26 @@ def cmd_serve(args: argparse.Namespace) -> int:
|
||||
else:
|
||||
_say(f"Port {DEFAULT_PORT} is in use; serving on {port} instead.")
|
||||
|
||||
# Bound before the credential is written, so an engine that cannot listen
|
||||
# leaves `client.json` naming the engine that is actually up rather than a
|
||||
# port somebody else holds.
|
||||
server = uvicorn.Server(
|
||||
uvicorn.Config(
|
||||
app,
|
||||
host=args.host,
|
||||
port=port,
|
||||
log_level=args.log_level,
|
||||
log_config=_log_config(args.log_level),
|
||||
)
|
||||
)
|
||||
try:
|
||||
sock = server.config.bind_socket()
|
||||
except SystemExit:
|
||||
# uvicorn has logged why, and exits rather than raising.
|
||||
if args.port is not None:
|
||||
_say(f" Port {port} was asked for; leave --port off to take a free one.")
|
||||
return 1
|
||||
|
||||
url = f"http://{reachable}:{port}"
|
||||
token_path = _sign_in(admin_id, url, data_dir)
|
||||
|
||||
@@ -600,19 +619,24 @@ def cmd_serve(args: argparse.Namespace) -> int:
|
||||
if _appended_log():
|
||||
# Started by the dashboard, with `serve.log` as this process's stdout.
|
||||
_watch_log()
|
||||
_hold_pidfile(
|
||||
pidfile,
|
||||
lambda: uvicorn.run(
|
||||
app,
|
||||
host=args.host,
|
||||
port=port,
|
||||
log_level=args.log_level,
|
||||
log_config=_log_config(args.log_level),
|
||||
),
|
||||
)
|
||||
_hold_pidfile(pidfile, lambda: _serve_on(server, sock))
|
||||
return 0
|
||||
|
||||
|
||||
def _serve_on(server: Any, sock: socket.socket) -> None:
|
||||
"""What `uvicorn.run` does for a single process, on a socket already bound."""
|
||||
from uvicorn.main import STARTUP_FAILURE
|
||||
|
||||
try:
|
||||
server.run(sockets=[sock])
|
||||
except KeyboardInterrupt:
|
||||
pass
|
||||
if not server.started:
|
||||
# The app never came up — a lifespan that raised — and the exit code
|
||||
# says so, as it did when uvicorn ran the whole thing.
|
||||
sys.exit(STARTUP_FAILURE)
|
||||
|
||||
|
||||
def _log_config(level: str) -> dict[str, Any]:
|
||||
"""Uvicorn's logging, with the engine's own loggers drawn the same way.
|
||||
|
||||
|
||||
@@ -935,6 +935,38 @@ def test_serve_moves_off_a_port_that_is_taken() -> None:
|
||||
assert DEFAULT_PORT == 8000
|
||||
|
||||
|
||||
def test_a_port_that_cannot_be_bound_writes_no_credential(tmp_path, monkeypatch):
|
||||
"""The token is written once the socket is ours, not on the way to it.
|
||||
|
||||
Written first, a failed start left `client.json` naming a port another
|
||||
project's engine holds — and every client here then talked to that one.
|
||||
"""
|
||||
import socket
|
||||
from types import SimpleNamespace
|
||||
|
||||
from fluksio import cli
|
||||
from fluksio.core import bootstrap
|
||||
|
||||
admin = SimpleNamespace(id="admin", email="admin@example.com")
|
||||
monkeypatch.setattr(cli, "_prepare", lambda data_dir: None)
|
||||
monkeypatch.setattr(bootstrap, "ensure_superuser", lambda *a, **k: (admin, None))
|
||||
# uvicorn's logging config would otherwise outlive this test.
|
||||
monkeypatch.setattr(cli, "_log_config", lambda level: None)
|
||||
|
||||
data_dir = tmp_path / ".fluksio"
|
||||
with socket.socket() as held:
|
||||
held.bind(("127.0.0.1", 0))
|
||||
held.listen(1)
|
||||
taken = held.getsockname()[1]
|
||||
args = cli._parser().parse_args(
|
||||
["serve", "--plain", "--port", str(taken), "--data-dir", str(data_dir)]
|
||||
)
|
||||
assert cli.cmd_serve(args) == 1
|
||||
|
||||
assert not (data_dir / "client.json").exists()
|
||||
assert not (data_dir / cli.PIDFILE).exists()
|
||||
|
||||
|
||||
def test_a_duration_reads_the_way_an_age_does() -> None:
|
||||
"""One notation for both, rather than one per place that printed one."""
|
||||
from fluksio.sdk.cli import _dur
|
||||
|
||||
+2
-1
@@ -53,7 +53,8 @@ The default port moves out of the way when something already has it (8001,
|
||||
8002, and so on) and says which one it took; the URL written to
|
||||
`client.json` is the one it is actually on. A port you *asked* for is never
|
||||
moved off: `--port 9000` on a taken 9000 fails, because something else is
|
||||
there and you named it.
|
||||
there and you named it. It fails before `client.json` is written, so every
|
||||
client keeps talking to the engine that is actually up.
|
||||
|
||||
What it will *not* do is start a second engine for the same instance. Before
|
||||
anything else, `serve` looks for an engine already serving this directory —
|
||||
|
||||
Reference in New Issue
Block a user