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

Log exception traceback in case of invalid HTTP request when using h11 #1945

Closed
wants to merge 1 commit into from
Closed

Conversation

ZhymabekRoman
Copy link

Author: florimondmanca (https://github.com/florimondmanca)
Source: #889
Revert PR: #1518

Motivation: I absolutely don't understand why this commit was reverted, if there are errors during execution you shouldn't keep quiet about them. Today I encountered the error Invalid HTTP request received. and I was stumped for hours because of it. After adding a traceback of the error it immediately became clear: h11._util.RemoteProtocolError: Missing mandatory Host: header. The funny thing about this error is that it appeared to me only if I used reverse proxy NGINX,

@euri10
Copy link
Member

euri10 commented Apr 15, 2023

Author: florimondmanca (https://github.com/florimondmanca) Source: #889 Revert PR: #1518

Motivation: I absolutely don't understand why this commit was reverted, if there are errors during execution you shouldn't keep quiet about them. Today I encountered the error Invalid HTTP request received. and I was stumped for hours because of it. After adding a traceback of the error it immediately became clear: h11._util.RemoteProtocolError: Missing mandatory Host: header. The funny thing about this error is that it appeared to me only if I used reverse proxy NGINX,

it is reverted because it serves no purpose anymore, it was added as a way for us to get the info needed to solve #344 which is now solved

I think a switch to opt-in for logging the warning could be considered, in which case we shoul dhave it for both h11 and httptools and not just h11 as it is now.

Just having the warning without opt-in is imho highly annoying to say the least, the default should be not to have it

@euri10 euri10 closed this Apr 15, 2023
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.

None yet

2 participants