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

Fix crash on exist with "--uds" if socket not exists #1725

Merged
merged 2 commits into from Oct 27, 2022
Merged

Fix crash on exist with "--uds" if socket not exists #1725

merged 2 commits into from Oct 27, 2022

Conversation

JGoutin
Copy link
Contributor

@JGoutin JGoutin commented Oct 26, 2022

Currently uvicorn may crash when exiting if run with the "--uds" option, and if Uvicorn exits before the socket creation (For instance if the application to run crash on statup).

This PR fix the issue by ignoring the exception in that case.

@Kludex
Copy link
Sponsor Member

Kludex commented Oct 26, 2022

Do you have a MRE?

@JGoutin
Copy link
Contributor Author

JGoutin commented Oct 26, 2022

Yes:

With this app (mre.py):

from starlette.applications import Starlette

app = Starlette()

@app.on_event("startup")
async def startup():
    raise RuntimeError

And the command uvicorn --uds=/tmp/uvicorn.sock mre:app

INFO:     Started server process [28861]
INFO:     Waiting for application startup.
ERROR:    Traceback (most recent call last):
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/starlette/routing.py", line 645, in lifespan
    async with self.lifespan_context(app):
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/starlette/routing.py", line 540, in __aenter__
    await self._router.startup()
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/starlette/routing.py", line 622, in startup
    await handler()
  File "/home/fedora/Documents/./mre.py", line 7, in startup
    raise RuntimeError
RuntimeError

ERROR:    Application startup failed. Exiting.
Traceback (most recent call last):
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/bin/uvicorn", line 8, in <module>
    sys.exit(main())
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/uvicorn/main.py", line 404, in main
    run(
  File "/home/fedora/.cache/pypoetry/virtualenvs/mre-Y0ZI6pHK-py3.10/lib/python3.10/site-packages/uvicorn/main.py", line 571, in run
    os.remove(config.uds)  # pragma: py-win32
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/uvicorn.sock'

@Kludex
Copy link
Sponsor Member

Kludex commented Oct 26, 2022

Thanks. I'll check later today. 🙏

Copy link
Sponsor Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Thanks @JGoutin ! 🎉

I've tweaked a bit one of the tests to make sure we'll not have any regressions here.

@Kludex Kludex mentioned this pull request Oct 27, 2022
13 tasks
@Kludex Kludex merged commit 5ba3f13 into encode:master Oct 27, 2022
@JGoutin JGoutin deleted the uds_remove_fix branch October 27, 2022 16:32
@@ -95,7 +95,6 @@ def test_cli_call_multiprocess_run() -> None:
def test_cli_uds(tmp_path: Path) -> None: # pragma: py-win32
runner = CliRunner()
uds_file = tmp_path / "uvicorn.sock"
uds_file.touch(exist_ok=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change has reduced coverage on this file. Added back in #1741

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Which line was reduced?

Copy link
Contributor

@iudeen iudeen Oct 28, 2022

Choose a reason for hiding this comment

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

Line 571 in main.py

due to this os.path.exists(config.uds)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Let's test both scenarios - I'm already on it.

Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Kludex added a commit that referenced this pull request Oct 29, 2022
Co-authored-by: Marcelo Trylesinski <marcelotryle@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants