Skip to content

Fix missing self.servers exception #1845

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

Closed

Conversation

Archmonger
Copy link

@Archmonger Archmonger commented Jan 12, 2023

If another webserver is currently parked on uvicorn's port, then this exception will occur:

Traceback (most recent call last):
  File "C:\Users\Markg\Documents\Repositories\idom-docs\.venv\lib\site-packages\idom\backend\default.py", line 38, in serve_development_app
    return await _default_implementation().serve_development_app(
  File "C:\Users\Markg\Documents\Repositories\idom-docs\.venv\lib\site-packages\idom\backend\starlette.py", line 76, in serve_development_app
    await serve_development_asgi(app, host, port, started)
  File "C:\Users\Markg\Documents\Repositories\idom-docs\.venv\lib\site-packages\idom\backend\_common.py", line 53, in serve_development_asgi
    await asyncio.wait_for(server.shutdown(), timeout=3)
  File "C:\Users\Markg\AppData\Local\Programs\Python\Python310\lib\asyncio\tasks.py", line 445, in wait_for
    return fut.result()
  File "C:\Users\Markg\Documents\Repositories\idom-docs\.venv\lib\site-packages\uvicorn\server.py", line 258, in shutdown
    for server in self.servers:
AttributeError: 'Server' object has no attribute 'servers'

This PR ensures self.servers is always initialized so the error can be handled gracefully.

@Kludex
Copy link
Member

Kludex commented Jan 12, 2023

What's your goal?

@Archmonger
Copy link
Author

Archmonger commented Jan 12, 2023

To prevent an unexpected exception from occuring if something is taking the requested Uvicorn port during startup.

Now, uvicorn will now properly log that the port is busy during the self-activated shutdown.

@Kludex
Copy link
Member

Kludex commented Jan 12, 2023

Do you have a snippet that demonstrates what you mean?

@Archmonger
Copy link
Author

Archmonger commented Jan 13, 2023

Not exactly a snippet, but the process I've done to create this exception is:

  1. Run another webserver on a specific port (ex. running mkdocs serve on port 8000)
  2. Run uvicorn on an arbitrary ASGI application using that specific port. In my case, I was utilizing IDOM to run uvicorn

@Kludex
Copy link
Member

Kludex commented Feb 6, 2023

I'd appreciate an MRE. 👀

@Archmonger
Copy link
Author

Sure I'll draft one in a bit

@Kludex Kludex added the waiting author Waiting for author's reply label Feb 13, 2023
@Kludex
Copy link
Member

Kludex commented Feb 13, 2023

Sure I'll draft one in a bit

Ok 👀

@Kludex Kludex added this to the Version 0.21.0 milestone Feb 13, 2023
@Archmonger
Copy link
Author

Archmonger commented Feb 13, 2023

Oops, thanks for the reminder.

MRE generated. See the readme for steps to reproduce.

https://github.com/Archmonger/uvicorn-server-exception


This issue can only be reproduced when uvicorn is run within asyncio.run(serve_development_asgi) in idom.

With the changes in this PR, everything seems to be handled gracefully.

@Kludex
Copy link
Member

Kludex commented Feb 13, 2023

So it cannot be reproduced with uvicorn standalone?

@Archmonger
Copy link
Author

Archmonger commented Feb 13, 2023

Not from my one quick test.

serve_development_asgi seems to be a basic asyncio runner though.

Not entirely sure how that's causing this problem, but it does look like uvicorn's Server.shutdown() is making some unsafe assumptions that self.servers always exists. But self.servers will only exist if at least one successful Server.startup() has taken place.

@Kludex
Copy link
Member

Kludex commented Feb 13, 2023

This is the first time I'm seeing this issue, and it doesn't look like is reproducible against uvicorn standalone... Even if it's one line, I need a proof that the issue is here, and I can only have that if it's possible to reproduce with only uvicorn.

@Archmonger
Copy link
Author

Archmonger commented Feb 13, 2023

No problem. I'm actually seeing that this issue is ultimately caused by this LOC in IDOM.

Although the variable safety issue still exists within Server.shutdown, it at least doesn't seem to be breaking things yet.

I will refactor the server running logic in IDOM.

@Archmonger Archmonger closed this Feb 13, 2023
@Archmonger Archmonger deleted the fix-self-servers-exception branch February 27, 2023 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting author Waiting for author's reply
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants