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

Remove --debug flag #1640

Merged
merged 2 commits into from Sep 14, 2022
Merged

Remove --debug flag #1640

merged 2 commits into from Sep 14, 2022

Conversation

Kludex
Copy link
Sponsor Member

@Kludex Kludex commented Sep 11, 2022

I've created #1190 some time ago, but at the time I didn't remember why I was creating that PR.

The goal of this PR is to remove the --debug flag. This flag is actually hidden, so to be aware of its existence, you need to check uvicorns source code.

There's also a conditional on the DebugMiddleware, that makes the flag only useful if the exception happens before the http.response.start is sent from the application.

Consider the application:

async def app(scope, send, receive):
    raise Exception("HAHAH")

What the --debug flag returns for the client is:

❯ http GET :8000/         
HTTP/1.1 500 Internal Server Error
Transfer-Encoding: chunked
content-type: text/plain; charset=utf-8
date: Sun, 11 Sep 2022 12:15:46 GMT
server: uvicorn

Traceback (most recent call last):
  File "/home/marcelo/Development/encode/uvicorn/uvicorn/middleware/debug.py", line 104, in __call__
    await self.app(scope, receive, inner_send)
  File "/home/marcelo/Development/test/./main.py", line 2, in app
    raise Exception("HAHAH")
Exception: HAHAH

Without the flag, we have the same log on the server side, but the client receives the normal:

❯ http GET :8000/
HTTP/1.1 500 Internal Server Error
Transfer-Encoding: chunked
connection: close
content-type: text/plain; charset=utf-8
date: Sun, 11 Sep 2022 12:26:05 GMT
server: uvicorn

Internal Server Error

If we are using a web framework that handles exceptions, like Starlette, the --debug flag is completely useless, as the application already sets the Internal Server Error by default, see:

from starlette.applications import Starlette
from starlette.routing import Route


async def endpoint(request):
    raise Exception("haha")


app = Starlette(routes=[Route("/", endpoint)])

With the above application, and the --debug flag set, we have the following on the client side:

❯ http GET :8000/
HTTP/1.1 500 Internal Server Error
content-length: 21
content-type: text/plain; charset=utf-8
date: Sun, 11 Sep 2022 12:28:52 GMT
server: uvicorn

Internal Server Error

To summarize: this flag is not used, it doesn't bring much benefit, and we have a 3 years old issue to remove it.

@Kludex Kludex requested a review from euri10 September 11, 2022 12:31
@Kludex Kludex mentioned this pull request Sep 11, 2022
5 tasks
euri10
euri10 approved these changes Sep 12, 2022
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

agreed, just need to fix the coverage, not sure why removing all that code makes it drop..

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 12, 2022

This is quite interesting actually... I have a different behavior on line 193 in the config.py file on two different python versions... I didn't expect that. There must be something there that I'm not aware... Which is not related to this PR.

I'll check later on.

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 14, 2022

I've rerun the pipeline and the line that was marked as not covered on Python 3.7 is now covered, and another line became uncovered... What's happening here... 😅

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 14, 2022

There's some random behavior on the reload_directory_structure for Python 3.7.

I'm dropping 0.04 coverage for this. It would be cool to increase this number to 100%. 😞

@Kludex
Copy link
Sponsor Member Author

Kludex commented Sep 14, 2022

agreed, just need to fix the coverage, not sure why removing all that code makes it drop..

Answering this... If you have 100 potatoes, and 10 are bad potatoes, 90% are good. If you eat 10 potatoes, you have 80 potatoes, 80/90 = 88.8% potatoes are good. Same idea when removing lines of code that are covered.

@Kludex Kludex merged commit 791c246 into master Sep 14, 2022
12 checks passed
@Kludex Kludex deleted the remove-debug-flag branch September 14, 2022 20:35
@edmorley
Copy link
Contributor

edmorley commented Oct 7, 2022

@Kludex Hi! Should the middleware file itself be deleted too? It's currently both unused and untested:
https://github.com/encode/uvicorn/blob/master/uvicorn/middleware/debug.py

@Kludex
Copy link
Sponsor Member Author

Kludex commented Oct 7, 2022

That's why the coverage dropped...

Yes. I missed that. Would you like to create a PR @edmorley ? 👀

edmorley added a commit to edmorley/uvicorn that referenced this pull request Oct 7, 2022
Since it's unused and untested as of encode#1640.
@edmorley edmorley mentioned this pull request Oct 7, 2022
edmorley added a commit to edmorley/uvicorn that referenced this pull request Oct 7, 2022
Since it's unused and untested as of encode#1640.
edmorley added a commit to edmorley/uvicorn that referenced this pull request Oct 7, 2022
Since it's unused and untested as of encode#1640.
Kludex added a commit to sephioh/uvicorn that referenced this pull request Oct 29, 2022
* Remove debug flag

* Drop 0.04 coverage
Kludex added a commit that referenced this pull request Oct 29, 2022
* Remove debug flag

* Drop 0.04 coverage
pschmitt added a commit to pschmitt/jcalapi that referenced this pull request Nov 9, 2022
jbruce-dp added a commit to TheDreamPort/rpe021-public that referenced this pull request Dec 2, 2022
See uvicorn pull request 1640: encode/uvicorn#1640
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Phase out debug in favour of reload
3 participants